-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, December 14, 2016 6:16 PM To: Anand, Jerome jerome.anand@intel.com Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; ville.syrjala@linux.intel.com; broonie@kernel.org; pierre- louis.bossart@linux.intel.com; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [PATCH 3/7] ALSA: add shell for Intel HDMI LPE audio driver
On Mon, 12 Dec 2016 19:10:39 +0100, Jerome Anand wrote:
On Baytrail and Cherrytrail, HDaudio may be fused out or disabled by the BIOS. This driver enables an alternate path to the i915 display registers and DMA.
Although there is no hardware path between i915 display and LPE/SST audio clusters, this HDMI capability is referred to in the documentation as "HDMI LPE Audio" so we keep the name for consistency. There is no hardware path or control dependencies with the LPE/SST DSP
functionality.
The hdmi-lpe-audio driver will be probed when the i915 driver creates a child platform device.
Since this driver is neither SoC nor PCI, a new x86 folder is added
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Jerome Anand jerome.anand@intel.com
Why do we need a "shell" and indirect calls? This is a small driver set, so it's not utterly unacceptable, but it still makes things a bit more complicated than necessary, so I'd like to understand the idea behind it.
This solution does not use component interface to talk between Audio and i915 driver. The reason for that is the HDMI audio device is created by i915 driver with only one callback pointer passed as pdata. Unlike HDA, the HDMI audio driver does not get loaded if the i915 does not create child device. Since there is only one callback needed we are not using the component interface to make things simpler. This design was co-worked with i915 folks to makes interaction simpler.
sound/Kconfig | 2 + sound/Makefile | 2 +- sound/x86/Kconfig | 16 + sound/x86/Makefile | 8 + sound/x86/intel_hdmi_lpe_audio.c | 622 +++++++++++++++++++++++++++++++++++ sound/x86/intel_hdmi_lpe_audio.h | 692 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 1341 insertions(+), 1 deletion(-) create mode 100644 sound/x86/Kconfig create mode 100644 sound/x86/Makefile create mode 100644 sound/x86/intel_hdmi_lpe_audio.c create mode 100644 sound/x86/intel_hdmi_lpe_audio.h
diff --git a/sound/Kconfig b/sound/Kconfig index 5a240e0..ee2e69a 100644 --- a/sound/Kconfig +++ b/sound/Kconfig @@ -108,6 +108,8 @@ source "sound/parisc/Kconfig"
source "sound/soc/Kconfig"
+source "sound/x86/Kconfig"
endif # SND
menuconfig SOUND_PRIME diff --git a/sound/Makefile b/sound/Makefile index c41bdf5..6de45d2 100644 --- a/sound/Makefile +++ b/sound/Makefile @@ -5,7 +5,7 @@ obj-$(CONFIG_SOUND) += soundcore.o obj-$(CONFIG_SOUND_PRIME) += oss/ obj-$(CONFIG_DMASOUND) += oss/ obj-$(CONFIG_SND) += core/ i2c/ drivers/ isa/ pci/ ppc/ arm/ sh/ synth/
usb/ \
- firewire/ sparc/ spi/ parisc/ pcmcia/ mips/ soc/ atmel/ hda/
- firewire/ sparc/ spi/ parisc/ pcmcia/ mips/ soc/ atmel/ hda/ x86/
obj-$(CONFIG_SND_AOA) += aoa/
# This one must be compilable even if sound is configured out diff --git a/sound/x86/Kconfig b/sound/x86/Kconfig new file mode 100644 index 0000000..182adf3 --- /dev/null +++ b/sound/x86/Kconfig @@ -0,0 +1,16 @@ +menuconfig SND_X86
- tristate "X86 sound devices"
- ---help---
X86 sound devices that don't fall under SoC or PCI categories+if SND_X86
+config HDMI_LPE_AUDIO
- tristate "HDMI audio without HDaudio on Intel Atom platforms"
- depends on DRM_I915
default nhelpChoose this option to support HDMI LPE Audio modePlease fix the indentation. Also a bit more description would be more user- friendly.
OK
+endif # SND_X86 diff --git a/sound/x86/Makefile b/sound/x86/Makefile new file mode 100644 index 0000000..78b2ae1 --- /dev/null +++ b/sound/x86/Makefile @@ -0,0 +1,8 @@ +DRIVER_NAME := hdmi_lpe_audio
+ccflags-y += -Idrivers/gpu/drm/i915
Is it just for intel_lpe_audio.h? Then rather put intel_lpe_audio.h to include/drm.
OK
+$(DRIVER_NAME)-objs += \
- intel_hdmi_lpe_audio.o
+obj-$(CONFIG_HDMI_LPE_AUDIO) += $(DRIVER_NAME).o
Don't use substitution, it's rather confusing. Also, we give "snd-" prefix for the sound driver in general. If no big reason against it, keep this rule please.
OK
--- /dev/null +++ b/sound/x86/intel_hdmi_lpe_audio.c
(snip)
@@ -0,0 +1,622 @@ +/*
- intel_hdmi_lpe_audio.c - Intel HDMI LPE audio driver for Atom
+platforms
- Copyright (C) 2016 Intel Corp
- Authors:
Jerome Anand <jerome.anand@intel.com>
Aravind Siddappaji <aravindx.siddappaji@intel.com>
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +~~~~~ > + * > + * 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. > + * > + * > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~+~~~~~
- */
+#define pr_fmt(fmt) "hdmi_lpe_audio: " fmt
Better to use dev_*() variant.
OK
+#include <linux/platform_device.h> +#include <linux/irqreturn.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/pci.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 <drm/intel_lpe_audio.h> +#include "intel_hdmi_lpe_audio.h"
+/* globals*/ +struct platform_device *gpdev; +int _hdmi_state; +union otm_hdmi_eld_t hdmi_eld;
For the global variables, give some prefix, as we have no proper namespace in C.
OK
And, what's the reason to keep copies of the state here? Can't we peek the pdata at each time?
OK
+struct hdmi_lpe_audio_ctx {
- int irq;
- void __iomem *mmio_start;
- had_event_call_back had_event_callbacks;
- struct snd_intel_had_interface *had_interface;
- void *had_pvt_data;
- int tmds_clock_speed;
- unsigned int had_config_offset;
- int hdmi_audio_interrupt_mask;
- struct work_struct hdmi_audio_wq;
+};
+static inline void hdmi_set_eld(void *eld) {
- int size = (sizeof(hdmi_eld)) > HDMI_MAX_ELD_BYTES ?
HDMI_MAX_ELD_BYTES :(sizeof(hdmi_eld));How can the size differ...? If any, it should be checked by BUILD_BUG_ON() or such.
OK
- memcpy((void *)&hdmi_eld, eld, size);
Also, when copying from the graphics side, isn't it safer to pass the given ELD size as well? Then we can zero-clear the rest bytes if it's short.
OK
+}
+static inline int hdmi_get_eld(void *eld) {
- memcpy(eld, (void *)&hdmi_eld, sizeof(hdmi_eld));
- {
int i;uint8_t *eld_data = (uint8_t *)&hdmi_eld;pr_debug("hdmi_get_eld:\n{{");for (i = 0; i < sizeof(hdmi_eld); i++)pr_debug("0x%x, ", eld_data[i]);pr_debug("}}\n");- }
There is a hexdump debug print helper, too.
OK
- return HAD_SUCCESS;
Let's use the normal return code, i.e. 0 or a negative error.
OK
+}
+static inline struct hdmi_lpe_audio_ctx *get_hdmi_context(void) {
- struct hdmi_lpe_audio_ctx *ctx;
- ctx = platform_get_drvdata(gpdev);
- return ctx;
+}
Hrm... Why not storing the audio pdev in i915 pdata? Then the notify callback can extract it easily.
The current audio driver interface implementation treats the input pdata as Read-only and I don't want to store any audio info in that.
(snip)
+static int hdmi_lpe_audio_probe(struct platform_device *pdev) {
- struct hdmi_lpe_audio_ctx *ctx;
- struct intel_hdmi_lpe_audio_pdata *pdata;
- int irq;
- struct resource *res_mmio;
- void __iomem *mmio_start;
- int ret = 0;
- unsigned long flag_irq;
- const struct pci_device_id cherryview_ids[] = {
Missing static.
OK
{PCI_DEVICE(0x8086, 0x22b0)},{PCI_DEVICE(0x8086, 0x22b1)},{PCI_DEVICE(0x8086, 0x22b2)},{PCI_DEVICE(0x8086, 0x22b3)},{}- };
- pr_debug("Enter %s\n", __func__);
- /*TBD:remove globals*/
- gpdev = pdev;
- _hdmi_state = hdmi_connector_status_disconnected;
- /* get resources */
- irq = platform_get_irq(pdev, 0);
- if (irq < 0) {
pr_err("Could not get irq resource\n");return -ENODEV;- }
- res_mmio = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res_mmio) {
pr_err("Could not get IO_MEM resources\n");return -ENXIO;- }
- pr_debug("%s: mmio_start = 0x%x, mmio_end = 0x%x\n", __func__,
(unsigned int)res_mmio->start, (unsigned int)res_mmio-end);
- mmio_start = ioremap_nocache(res_mmio->start,
(size_t)((res_mmio->end - res_mmio->start)+ 1));- if (!mmio_start) {
pr_err("Could not get ioremap\n");return -EACCES;- }
- /* setup interrupt handler */
- ret = request_irq(irq, display_pipe_interrupt_handler,
0, /* FIXME: is IRQF_SHARED needed ? */It's a irqchip, so no sharing is considered, right? Then IRQF_SHARED is rather wrong.
OK
--- /dev/null +++ b/sound/x86/intel_hdmi_lpe_audio.h
(snip)
+#define HMDI_LPE_AUDIO_DRIVER_NAME "intel-hdmi-lpe-
audio"
+#define HAD_DRIVER_VERSION "0.01.003"
The driver version string doesn't make sense in most cases for in-kernel codes.
OK - will be removed
+#define HAD_MAX_DEVICES 1 +#define HAD_MIN_CHANNEL 2 +#define HAD_MAX_CHANNEL 8 +#define HAD_NUM_OF_RING_BUFS 4
+/* HDMI Audio LPE Error Codes */ +#define HAD_SUCCESS 0 +#define HAD_FAIL 1
Avoid the own definition.
OK
+/* Assume 192KHz, 8channel, 25msec period */ +#define HAD_MAX_BUFFER (600*1024) +#define HAD_MIN_BUFFER (32*1024) +#define HAD_MAX_PERIODS 4 +#define HAD_MIN_PERIODS 4 +#define HAD_MAX_PERIOD_BYTES
(HAD_MAX_BUFFER/HAD_MIN_PERIODS)
+#define HAD_MIN_PERIOD_BYTES 256 +#define HAD_FIFO_SIZE 0 /* fifo not being used */ +#define MAX_SPEAKERS 8 +/* TODO: Add own tlv when channel map is ported for user space */ +#define USE_ALSA_DEFAULT_TLV
+#define AUD_SAMPLE_RATE_32 32000 +#define AUD_SAMPLE_RATE_44_1 44100 +#define AUD_SAMPLE_RATE_48 48000 +#define AUD_SAMPLE_RATE_88_2 88200 +#define AUD_SAMPLE_RATE_96 96000 +#define AUD_SAMPLE_RATE_176_4 176400 +#define AUD_SAMPLE_RATE_192 192000
+#define HAD_MIN_RATE AUD_SAMPLE_RATE_32 +#define HAD_MAX_RATE AUD_SAMPLE_RATE_192
+#define DIS_SAMPLE_RATE_25_2 25200 +#define DIS_SAMPLE_RATE_27 27000 +#define DIS_SAMPLE_RATE_54 54000 +#define DIS_SAMPLE_RATE_74_25 74250 +#define DIS_SAMPLE_RATE_148_5 148500 +#define HAD_REG_WIDTH 0x08 +#define HAD_MAX_HW_BUFS 0x04 +#define HAD_MAX_DIP_WORDS 16 +#define INTEL_HAD "IntelHdmiLpeAudio"
+/* _AUD_CONFIG register MASK */ +#define AUD_CONFIG_MASK_UNDERRUN 0xC0000000 +#define AUD_CONFIG_MASK_SRDBG 0x00000002 +#define AUD_CONFIG_MASK_FUNCRST 0x00000001
+#define MAX_CNT 0xFF +#define HAD_SUSPEND_DELAY 1000
+#define OTM_HDMI_ELD_SIZE 128
+union otm_hdmi_eld_t {
- uint8_t eld_data[OTM_HDMI_ELD_SIZE];
- #pragma pack(1)
Use __packed notion for the packed structs.
OK
- struct {
/* Byte[0] = ELD Version Number */union {uint8_t byte0;struct {uint8_t reserved:3; /* Reserf */uint8_t eld_ver:5; /* ELD Version Number *//* 00000b - reserved* 00001b - first rev, obsoleted* 00010b - version 2, supporting CEA version* 861D or below* 00011b:11111b - reserved* for future*/};};/* Byte[1] = Vendor Version Field */union {uint8_t vendor_version;struct {uint8_t reserved1:3;uint8_t veld_ver:5; /* Version number of theELD
* extension. This value is* provisioned and uniqueto
* each vendor.*/};};/* Byte[2] = Baseline Length field */uint8_t baseline_eld_length; /* Length of the Baselinestructure
* divided by Four.*//* Byte [3] = Reserved for future use */uint8_t byte3;/* Starting of the BaseLine EELD structure* Byte[4] = Monitor Name Length*/union {uint8_t byte4;struct {uint8_t mnl:5;uint8_t cea_edid_rev_id:3;};};/* Byte[5] = Capabilities */union {uint8_t capabilities;struct {uint8_t hdcp:1; /* HDCP support */uint8_t ai_support:1; /* AI support */uint8_t connection_type:2; /* Connectiontype
* 00 - HDMI* 01 - DP* 10 -11 Reserved* for future* connection types*/uint8_t sadc:4; /* Indicates number of 3bytes
* Short Audio Descriptors.*/};};/* Byte[6] = Audio Synch Delay */uint8_t audio_synch_delay; /* Amount of time reported bythe
* sink that the video trails audio* in milliseconds.*//* Byte[7] = Speaker Allocation Block */union {uint8_t speaker_allocation_block;struct {uint8_t flr:1; /*Front Left and Rightchannels*/
uint8_t lfe:1; /*Low Frequency Effectchannel*/
uint8_t fc:1; /*Center transmissionchannel*/
uint8_t rlr:1; /*Rear Left and Right channels*/uint8_t rc:1; /*Rear Center channel*/uint8_t flrc:1; /*Front left and Right of Center*transmission channels*/uint8_t rlrc:1; /*Rear left and Right of Center*transmission channels*/uint8_t reserved3:1; /* Reserved */};};/* Byte[8 - 15] - 8 Byte port identification value */uint8_t port_id_value[8];/* Byte[16 - 17] - 2 Byte Manufacturer ID */uint8_t manufacturer_id[2];/* Byte[18 - 19] - 2 Byte Product ID */uint8_t product_id[2];/* Byte [20-83] - 64 Bytes of BaseLine Data */uint8_t mn_sand_sads[64]; /* This will include* - ASCII string of Monitor name* - List of 3 byte SADs* - Zero padding*//* Vendor ELD Block should continue here!* No Vendor ELD block defined as of now.*/- };
- #pragma pack()
+};
Well, we should have a single definition of ELD somewhere. We can keep this locally until we have some unified header, though.
OK
In anyway, better to avoid uint8_t.
OK I'll use unsigned char
thanks,
Takashi