Re: [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI LPE audio driver

Re: [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI LPE audio driver

[Ville Syrjälä]

On Fri, Nov 25, 2016 at 05:25:42AM +0530, Jerome Anand wrote:
> Enable support for HDMI LPE audio mode on Baytrail and
> Cherrytrail when HDaudio controller is not detected
> 
> Setup minimum required resources during i915_driver_load:
> 1. Create a platform device to share MMIO/IRQ resources
> 2. Make the platform device child of i915 device for runtime PM.
> 3. Create IRQ chip to forward HDMI LPE audio irqs.
> 
> HDMI LPE audio driver (a standalone sound driver) probes the
> LPE audio device and creates a new sound card.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> ---
>  Documentation/gpu/i915.rst             |   9 +
>  drivers/gpu/drm/i915/Makefile          |   3 +
>  drivers/gpu/drm/i915/i915_drv.c        |   8 +-
>  drivers/gpu/drm/i915/i915_drv.h        |  15 ++
>  drivers/gpu/drm/i915/i915_irq.c        |  29 +++
>  drivers/gpu/drm/i915/i915_reg.h        |   3 +
>  drivers/gpu/drm/i915/intel_lpe_audio.c | 369 +++++++++++++++++++++++++++++++++
>  include/drm/intel_lpe_audio.h          |  45 ++++
>  8 files changed, 479 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_lpe_audio.c
>  create mode 100644 include/drm/intel_lpe_audio.h
> 
> diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> index 117d2ab..a671eee 100644
> --- a/Documentation/gpu/i915.rst
> +++ b/Documentation/gpu/i915.rst
> @@ -213,6 +213,15 @@ Video BIOS Table (VBT)
>  .. kernel-doc:: drivers/gpu/drm/i915/intel_vbt_defs.h
>     :internal:
>  
> +intel hdmi lpe audio support
> +----------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/i915/intel_lpe_audio.c
> +   :doc:  LPE Audio integration for HDMI or DP playback
> +
> +.. kernel-doc:: drivers/gpu/drm/i915/intel_lpe_audio.c
> +   :internal:
> +
>  Memory Management and Command Submission
>  ========================================
>  
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 580602d..3a2369c 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -126,6 +126,9 @@ i915-y += intel_gvt.o
>  include $(src)/gvt/Makefile
>  endif
>  
> +# LPE Audio for VLV and CHT
> +i915-y += intel_lpe_audio.o
> +
>  obj-$(CONFIG_DRM_I915) += i915.o
>  
>  CFLAGS_i915_trace_points.o := -I$(src)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b893e67..93811ed 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1144,7 +1144,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>  	if (IS_GEN5(dev_priv))
>  		intel_gpu_ips_init(dev_priv);
>  
> -	i915_audio_component_init(dev_priv);
> +	if (intel_lpe_audio_init(dev_priv) < 0)
> +		i915_audio_component_init(dev_priv);
>  
>  	/*
>  	 * Some ports require correctly set-up hpd registers for detection to
> @@ -1162,7 +1163,10 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>   */
>  static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  {
> -	i915_audio_component_cleanup(dev_priv);
> +	if (HAS_LPE_AUDIO(dev_priv))
> +		intel_lpe_audio_teardown(dev_priv);
> +	else
> +		i915_audio_component_cleanup(dev_priv);
>  
>  	intel_gpu_ips_teardown();
>  	acpi_video_unregister();
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 970e50b..2a79048 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2284,6 +2284,12 @@ struct drm_i915_private {
>  	/* Used to save the pipe-to-encoder mapping for audio */
>  	struct intel_encoder *av_enc_map[I915_MAX_PIPES];
>  
> +	/* necessary resource sharing with HDMI LPE audio driver. */
> +	struct {
> +		struct platform_device *platdev;
> +		int	irq;
> +	} lpe_audio;
> +
>  	/*
>  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>  	 * will be rejected. Instead look for a better place.
> @@ -2773,6 +2779,8 @@ intel_info(const struct drm_i915_private *dev_priv)
>  
>  #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
>  
> +#define HAS_LPE_AUDIO(dev_priv) ((dev_priv)->lpe_audio.platdev != NULL)
> +
>  #define INTEL_PCH_DEVICE_ID_MASK		0xff00
>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
>  #define INTEL_PCH_CPT_DEVICE_ID_TYPE		0x1c00
> @@ -3547,6 +3555,13 @@ extern int i915_restore_state(struct drm_device *dev);
>  void i915_setup_sysfs(struct drm_i915_private *dev_priv);
>  void i915_teardown_sysfs(struct drm_i915_private *dev_priv);
>  
> +/* i915_lpe_audio.c */
> +int  intel_lpe_audio_init(struct drm_i915_private *dev_priv);
> +int  intel_lpe_audio_setup(struct drm_i915_private *dev_priv);
> +void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv);
> +void intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv);
> +bool intel_lpe_audio_detect(struct drm_i915_private *dev_priv);
> +
>  /* intel_i2c.c */
>  extern int intel_setup_gmbus(struct drm_device *dev);
>  extern void intel_teardown_gmbus(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 07ca71c..6c98b28 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1894,6 +1894,15 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>  
>  		/*
> +		 * LPE audio interrupts are only enabled on Baytrail and
> +		 * CherryView platforms without HDaudio
> +		 */
> +		if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> +			   I915_LPE_PIPE_B_INTERRUPT |
> +			   I915_LPE_PIPE_C_INTERRUPT))

No pipe C on vlv.

> +			intel_lpe_audio_irq_handler(dev_priv);
> +
> +		/*
>  		 * VLV_IIR is single buffered, and reflects the level
>  		 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
>  		 */
> @@ -1974,6 +1983,15 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>  
>  		/*
> +		 * LPE audio interrupts are only enabled on Baytrail and
> +		 * CherryView platforms without HDaudio
> +		 */
> +		if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> +			   I915_LPE_PIPE_B_INTERRUPT |
> +			   I915_LPE_PIPE_C_INTERRUPT))
> +			intel_lpe_audio_irq_handler(dev_priv);
> +
> +		/*
>  		 * VLV_IIR is single buffered, and reflects the level
>  		 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
>  		 */
> @@ -2930,6 +2948,17 @@ static void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
>  
>  	WARN_ON(dev_priv->irq_mask != ~0);
>  
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {

This is vlv/chv specific code, so no need to check for it.

> +		/* add interrupt masks unconditially here, the actual unmask
> +		 * will take place only if the LPE_AUDIO mode is detected
> +		 */
> +		u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> +			I915_LPE_PIPE_B_INTERRUPT |
> +			I915_LPE_PIPE_C_INTERRUPT);
> +
> +		enable_mask |= val;
> +	}
> +
>  	dev_priv->irq_mask = ~enable_mask;
>  
>  	GEN5_IRQ_INIT(VLV_, dev_priv->irq_mask, enable_mask);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index fcad8fa..6b16ed0 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2395,6 +2395,9 @@ enum skl_disp_power_wells {
>  #define I915_ASLE_INTERRUPT				(1<<0)
>  #define I915_BSD_USER_INTERRUPT				(1<<25)
>  
> +#define I915_HDMI_LPE_AUDIO_BASE	(VLV_DISPLAY_BASE + 0x65000)
> +#define I915_HDMI_LPE_AUDIO_SIZE	0x1000
> +
>  #define GEN6_BSD_RNCID			_MMIO(0x12198)
>  
>  #define GEN7_FF_THREAD_MODE		_MMIO(0x20a0)
> diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> new file mode 100644
> index 0000000..5335fc6
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> @@ -0,0 +1,369 @@
> +/*
> + * Copyright © 2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> + *    Jerome Anand <jerome.anand@intel.com>
> + *    based on VED patches
> + *
> + */
> +
> +/**
> + * DOC: LPE Audio integration for HDMI or DP playback
> + *
> + * Motivation:
> + * Atom platforms (e.g. valleyview and cherryTrail) integrates a DMA-based
> + * interface as an alternative to the traditional HDaudio path. While this
> + * mode is unrelated to the LPE aka SST audio engine, the documentation refers
> + * to this mode as LPE so we keep this notation for the sake of consistency.
> + *
> + * The interface is handled by a separate standalone driver maintained in the
> + * ALSA subsystem for simplicity. To minimize the interaction between the two
> + * subsystems, a bridge is setup between the hdmi-lpe-audio and i915:
> + * 1. Create a platform device to share MMIO/IRQ resources
> + * 2. Make the platform device child of i915 device for runtime PM.
> + * 3. Create IRQ chip to forward the LPE audio irqs.
> + * the hdmi-lpe-audio driver probes the lpe audio device and creates a new
> + * sound card
> + *
> + * Threats:
> + * Due to the restriction in Linux platform device model, user need manually
> + * uninstall the hdmi-lpe-audio driver before uninstalling i915 module,
> + * otherwise we might run into use-after-free issues after i915 removes the
> + * platform device: even though hdmi-lpe-audio driver is released, the modules
> + * is still in "installed" status.
> + *
> + * Implementation:
> + * The MMIO/REG platform resources are created according to the registers
> + * specification.
> + * When forwarding LPE audio irqs, the flow control handler selection depends
> + * on the platform, for example on valleyview handle_simple_irq is enough.
> + *
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/pci.h>
> +
> +#include "i915_drv.h"
> +#include <linux/delay.h>
> +#include <drm/intel_lpe_audio.h>
> +
> +static struct platform_device*
> +lpe_audio_platdev_create(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = &dev_priv->drm;
> +	int ret;
> +	struct resource rsc[2] = {};
> +	struct platform_device *platdev;
> +	u64 *dma_mask;
> +	struct intel_hdmi_lpe_audio_pdata *pdata;
> +
> +	if (dev_priv->lpe_audio.irq < 0)
> +		return ERR_PTR(-EINVAL);
> +
> +	platdev = platform_device_alloc("hdmi-lpe-audio", -1);
> +	if (!platdev) {
> +		ret = -ENOMEM;
> +		DRM_ERROR("Failed to allocate LPE audio platform device\n");
> +		goto err;
> +	}
> +
> +	/* to work-around check_addr in nommu_map_sg() */
> +	dma_mask = kmalloc(sizeof(*platdev->dev.dma_mask), GFP_KERNEL);
> +	if (!dma_mask) {
> +		ret = -ENOMEM;
> +		DRM_ERROR("Failed to allocate dma_mask\n");
> +		goto err_put_dev;
> +	}
> +	*dma_mask = DMA_BIT_MASK(32);
> +	platdev->dev.dma_mask = dma_mask;
> +	platdev->dev.coherent_dma_mask = *dma_mask;
> +
> +	rsc[0].start    = rsc[0].end = dev_priv->lpe_audio.irq;
> +	rsc[0].flags    = IORESOURCE_IRQ;
> +	rsc[0].name     = "hdmi-lpe-audio-irq";
> +
> +	rsc[1].start    = pci_resource_start(dev->pdev, 0) +
> +		I915_HDMI_LPE_AUDIO_BASE;
> +	rsc[1].end      = pci_resource_start(dev->pdev, 0) +
> +		I915_HDMI_LPE_AUDIO_BASE + I915_HDMI_LPE_AUDIO_SIZE - 1;
> +	rsc[1].flags    = IORESOURCE_MEM;
> +	rsc[1].name     = "hdmi-lpe-audio-mmio";
> +
> +	ret = platform_device_add_resources(platdev, rsc, 2);
> +	if (ret) {
> +		DRM_ERROR("Failed to add resource for platform device: %d\n",
> +			ret);
> +		goto err_put_dev;
> +	}
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +
> +	if (pdata == NULL) {
> +		ret = -ENOMEM;
> +		goto err_put_dev;
> +	}
> +	platdev->dev.platform_data = pdata;
> +	spin_lock_init(&pdata->lpe_audio_slock);
> +
> +	/* for LPE audio driver's runtime-PM */
> +	platdev->dev.parent = dev->dev;
> +	ret = platform_device_add(platdev);
> +	if (ret) {
> +		DRM_ERROR("Failed to add LPE audio platform device: %d\n",
> +			ret);
> +		goto err_put_dev;
> +	}
> +
> +	return platdev;
> +err_put_dev:
> +	platform_device_put(platdev);
> +	kfree(dma_mask);
> +err:
> +	return ERR_PTR(ret);
> +}
> +
> +static void lpe_audio_platdev_destroy(struct drm_i915_private *dev_priv)
> +{
> +	if (!HAS_LPE_AUDIO(dev_priv))
> +		return;
> +	platform_device_unregister(dev_priv->lpe_audio.platdev);
> +	kfree(dev_priv->lpe_audio.platdev->dev.dma_mask);
> +}
> +
> +static void lpe_audio_irq_unmask(struct irq_data *d)
> +{
> +	struct drm_device *dev = d->chip_data;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	unsigned long irqflags;
> +	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> +		I915_LPE_PIPE_B_INTERRUPT |
> +		I915_LPE_PIPE_C_INTERRUPT);

No pipe C on vlv.

> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +
> +	/*
> +	 * VLV_IER is already set in the vlv_display_postinstall(),
> +	 * we only change VLV_IIR and VLV_IMR
> +	 */
> +	dev_priv->irq_mask &= ~val;
> +	I915_WRITE(VLV_IIR, val);
> +	I915_WRITE(VLV_IIR, val);
> +	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> +	POSTING_READ(VLV_IMR);
> +
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +}
> +
> +static void lpe_audio_irq_mask(struct irq_data *d)
> +{
> +	struct drm_device *dev = d->chip_data;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	unsigned long irqflags;
> +	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> +		I915_LPE_PIPE_B_INTERRUPT |
> +		I915_LPE_PIPE_C_INTERRUPT);

ditto

> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +
> +	/*
> +	 * VLV_IER is already set in the vlv_display_postinstall(),
> +	 * we only change VLV_IIR and VLV_IMR
> +	 */
> +	dev_priv->irq_mask |= val;
> +	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> +	I915_WRITE(VLV_IIR, val);
> +	I915_WRITE(VLV_IIR, val);
> +	POSTING_READ(VLV_IIR);
> +
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +}
> +
> +static struct irq_chip lpe_audio_irqchip = {
> +	.name = "hdmi_lpe_audio_irqchip",
> +	.irq_mask = lpe_audio_irq_mask,
> +	.irq_unmask = lpe_audio_irq_unmask,
> +};
> +
> +static int lpe_audio_irq_init(struct drm_i915_private *dev_priv)
> +{
> +	int irq = dev_priv->lpe_audio.irq;
> +
> +	WARN_ON(!intel_irqs_enabled(dev_priv));
> +	irq_set_chip_and_handler_name(irq,
> +				&lpe_audio_irqchip,
> +				handle_simple_irq,
> +				"hdmi_lpe_audio_irq_handler");
> +
> +	return irq_set_chip_data(irq, &dev_priv->drm);

Just pass dev_priv as the chip data.

> +}
> +
> +/**
> + * intel_lpe_audio_irq_handler() - forwards the LPE audio irq
> + * @dev_priv: the i915 drm device private data
> + *
> + * the LPE Audio irq is forwarded to the irq handler registered by LPE audio
> + * driver.
> + */
> +void intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +
> +	ret = generic_handle_irq(dev_priv->lpe_audio.irq);
> +	if (ret)
> +		DRM_ERROR_RATELIMITED("error handling LPE audio irq: %d\n",
> +				ret);
> +}
> +
> +/**
> + * intel_lpe_audio_detect() - check & setup lpe audio if present
> + * @dev_priv: the i915 drm device private data
> + *
> + * Detect if lpe audio is present
> + *
> + * Return: true if lpe audio present else Return = false
> + */
> +bool intel_lpe_audio_detect(struct drm_i915_private *dev_priv)
> +{
> +	int lpe_present = false;
> +
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {

Could avoid indenting the whole thig by reversing this test and doing an
early return.

> +		static const struct pci_device_id atom_hdaudio_ids[] = {
> +			/* Baytrail */
> +			{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x0f04)},
> +			/* Braswell */
> +			{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x2284)},
> +			{}
> +		};

IIRC I asked if we could use a HDA class match instead. But I forget
what the answer was. No presumably?

> +
> +		if (!pci_dev_present(atom_hdaudio_ids)) {
> +			DRM_INFO("%s%s\n", "HDaudio controller not detected,",
> +				"using LPE audio instead\n");
> +			lpe_present = true;
> +		}
> +	}
> +	return lpe_present;
> +}
> +
> +/**
> + * intel_lpe_audio_setup() - setup the bridge between HDMI LPE Audio
> + * driver and i915
> + * @dev_priv: the i915 drm device private data
> + *
> + * set up the minimum required resources for the bridge: irq chip,
> + * platform resource and platform device. i915 device is set as parent
> + * of the new platform device.
> + *
> + * Return: 0 if successful. non-zero if allocation/initialization fails
> + */
> +int intel_lpe_audio_setup(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +
> +	dev_priv->lpe_audio.irq = irq_alloc_desc(0);
> +	if (dev_priv->lpe_audio.irq < 0) {
> +		DRM_ERROR("Failed to allocate IRQ desc: %d\n",
> +			dev_priv->lpe_audio.irq);
> +		ret = dev_priv->lpe_audio.irq;
> +		goto err;
> +	}
> +
> +	DRM_DEBUG("irq = %d\n", dev_priv->lpe_audio.irq);
> +
> +	ret = lpe_audio_irq_init(dev_priv);
> +
> +	if (ret) {
> +		DRM_ERROR("Failed to initialize irqchip for lpe audio: %d\n",
> +			ret);
> +		goto err_free_irq;
> +	}
> +
> +	dev_priv->lpe_audio.platdev = lpe_audio_platdev_create(dev_priv);
> +
> +	if (IS_ERR(dev_priv->lpe_audio.platdev)) {
> +		ret = PTR_ERR(dev_priv->lpe_audio.platdev);
> +		DRM_ERROR("Failed to create lpe audio platform device: %d\n",
> +			ret);
> +		goto err_free_irq;
> +	}
> +
> +	return 0;
> +err_free_irq:
> +	irq_free_desc(dev_priv->lpe_audio.irq);
> +err:
> +	dev_priv->lpe_audio.irq = -1;
> +	dev_priv->lpe_audio.platdev = NULL;
> +	return ret;
> +}
> +
> +
> +/**
> + * intel_lpe_audio_init() - detect and setup the bridge between HDMI LPE Audio
> + * driver and i915
> + * @dev_priv: the i915 drm device private data
> + *
> + * Return: 0 if successful. non-zero if detection or
> + * llocation/initialization fails
> + */
> +int intel_lpe_audio_init(struct drm_i915_private *dev_priv)
> +{
> +	int ret = -ENODEV;
> +
> +	if (intel_lpe_audio_detect(dev_priv)) {
> +		ret = intel_lpe_audio_setup(dev_priv);
> +		if (ret < 0)
> +			DRM_ERROR("failed to setup LPE Audio bridge\n");
> +	}
> +	return ret;
> +}
> +
> +/**
> + * intel_lpe_audio_teardown() - destroy the bridge between HDMI LPE
> + * audio driver and i915
> + * @dev_priv: the i915 drm device private data
> + *
> + * release all the resources for LPE audio <-> i915 bridge.
> + */
> +void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
> +{
> +	unsigned long irqflags;
> +	struct irq_desc *desc;
> +
> +	desc = (dev_priv->lpe_audio.irq >= 0) ?
> +		irq_to_desc(dev_priv->lpe_audio.irq) : NULL;

Why not just "if (lpe not initialized) return;" ?

> +
> +	/**
> +	 * mask LPE audio irq before destroying
> +	 */
> +	if (desc)
> +		lpe_audio_irq_mask(&desc->irq_data);

This seems racy. Should unregister the platdev first I think,

> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +
> +	lpe_audio_platdev_destroy(dev_priv);
> +
> +	if (desc)
> +		irq_free_desc(dev_priv->lpe_audio.irq);
> +
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +}
> diff --git a/include/drm/intel_lpe_audio.h b/include/drm/intel_lpe_audio.h
> new file mode 100644
> index 0000000..a64c449
> --- /dev/null
> +++ b/include/drm/intel_lpe_audio.h
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright © 2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef _INTEL_LPE_AUDIO_H_
> +#define _INTEL_LPE_AUDIO_H_
> +
> +#include <linux/types.h>
> +
> +#define HDMI_MAX_ELD_BYTES	128
> +
> +struct intel_hdmi_lpe_audio_eld {
> +	int port_id;
> +	unsigned char eld_data[HDMI_MAX_ELD_BYTES];
> +};
> +
> +struct intel_hdmi_lpe_audio_pdata {
> +	bool notify_pending;
> +	int tmds_clock_speed;
> +	bool hdmi_connected;
> +	struct intel_hdmi_lpe_audio_eld eld;
> +	void (*notify_audio_lpe)(void *audio_ptr);
> +	spinlock_t lpe_audio_slock;
> +};
> +
> +#endif /* _I915_LPE_AUDIO_H_ */
> -- 
> 2.9.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI LPE audio driver

[Anand, Jerome]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Thursday, November 24, 2016 7:02 PM
> To: Anand, Jerome <jerome.anand@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org;
> broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com;
> Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> Subject: Re: [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI LPE audio
> driver
> 
> On Fri, Nov 25, 2016 at 05:25:42AM +0530, Jerome Anand wrote:
> > Enable support for HDMI LPE audio mode on Baytrail and Cherrytrail
> > when HDaudio controller is not detected
> >
> > Setup minimum required resources during i915_driver_load:
> > 1. Create a platform device to share MMIO/IRQ resources 2. Make the
> > platform device child of i915 device for runtime PM.
> > 3. Create IRQ chip to forward HDMI LPE audio irqs.
> >
> > HDMI LPE audio driver (a standalone sound driver) probes the LPE audio
> > device and creates a new sound card.
> >
> > Signed-off-by: Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com>
> > Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> > ---
> >  Documentation/gpu/i915.rst             |   9 +
> >  drivers/gpu/drm/i915/Makefile          |   3 +
> >  drivers/gpu/drm/i915/i915_drv.c        |   8 +-
> >  drivers/gpu/drm/i915/i915_drv.h        |  15 ++
> >  drivers/gpu/drm/i915/i915_irq.c        |  29 +++
> >  drivers/gpu/drm/i915/i915_reg.h        |   3 +
> >  drivers/gpu/drm/i915/intel_lpe_audio.c | 369
> +++++++++++++++++++++++++++++++++
> >  include/drm/intel_lpe_audio.h          |  45 ++++
> >  8 files changed, 479 insertions(+), 2 deletions(-)  create mode
> > 100644 drivers/gpu/drm/i915/intel_lpe_audio.c
> >  create mode 100644 include/drm/intel_lpe_audio.h
> >
> > diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> > index 117d2ab..a671eee 100644
> > --- a/Documentation/gpu/i915.rst
> > +++ b/Documentation/gpu/i915.rst
> > @@ -213,6 +213,15 @@ Video BIOS Table (VBT)  .. kernel-doc::
> > drivers/gpu/drm/i915/intel_vbt_defs.h
> >     :internal:
> >
> > +intel hdmi lpe audio support
> > +----------------------------
> > +
> > +.. kernel-doc:: drivers/gpu/drm/i915/intel_lpe_audio.c
> > +   :doc:  LPE Audio integration for HDMI or DP playback
> > +
> > +.. kernel-doc:: drivers/gpu/drm/i915/intel_lpe_audio.c
> > +   :internal:
> > +
> >  Memory Management and Command Submission
> > ========================================
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile
> > b/drivers/gpu/drm/i915/Makefile index 580602d..3a2369c 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -126,6 +126,9 @@ i915-y += intel_gvt.o  include $(src)/gvt/Makefile
> > endif
> >
> > +# LPE Audio for VLV and CHT
> > +i915-y += intel_lpe_audio.o
> > +
> >  obj-$(CONFIG_DRM_I915) += i915.o
> >
> >  CFLAGS_i915_trace_points.o := -I$(src) diff --git
> > a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index b893e67..93811ed 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1144,7 +1144,8 @@ static void i915_driver_register(struct
> drm_i915_private *dev_priv)
> >  	if (IS_GEN5(dev_priv))
> >  		intel_gpu_ips_init(dev_priv);
> >
> > -	i915_audio_component_init(dev_priv);
> > +	if (intel_lpe_audio_init(dev_priv) < 0)
> > +		i915_audio_component_init(dev_priv);
> >
> >  	/*
> >  	 * Some ports require correctly set-up hpd registers for detection
> > to @@ -1162,7 +1163,10 @@ static void i915_driver_register(struct
> drm_i915_private *dev_priv)
> >   */
> >  static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> > {
> > -	i915_audio_component_cleanup(dev_priv);
> > +	if (HAS_LPE_AUDIO(dev_priv))
> > +		intel_lpe_audio_teardown(dev_priv);
> > +	else
> > +		i915_audio_component_cleanup(dev_priv);
> >
> >  	intel_gpu_ips_teardown();
> >  	acpi_video_unregister();
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index 970e50b..2a79048 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2284,6 +2284,12 @@ struct drm_i915_private {
> >  	/* Used to save the pipe-to-encoder mapping for audio */
> >  	struct intel_encoder *av_enc_map[I915_MAX_PIPES];
> >
> > +	/* necessary resource sharing with HDMI LPE audio driver. */
> > +	struct {
> > +		struct platform_device *platdev;
> > +		int	irq;
> > +	} lpe_audio;
> > +
> >  	/*
> >  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your
> patch
> >  	 * will be rejected. Instead look for a better place.
> > @@ -2773,6 +2779,8 @@ intel_info(const struct drm_i915_private
> > *dev_priv)
> >
> >  #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
> >
> > +#define HAS_LPE_AUDIO(dev_priv) ((dev_priv)->lpe_audio.platdev !=
> > +NULL)
> > +
> >  #define INTEL_PCH_DEVICE_ID_MASK		0xff00
> >  #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
> >  #define INTEL_PCH_CPT_DEVICE_ID_TYPE		0x1c00
> > @@ -3547,6 +3555,13 @@ extern int i915_restore_state(struct drm_device
> > *dev);  void i915_setup_sysfs(struct drm_i915_private *dev_priv);
> > void i915_teardown_sysfs(struct drm_i915_private *dev_priv);
> >
> > +/* i915_lpe_audio.c */
> > +int  intel_lpe_audio_init(struct drm_i915_private *dev_priv); int
> > +intel_lpe_audio_setup(struct drm_i915_private *dev_priv); void
> > +intel_lpe_audio_teardown(struct drm_i915_private *dev_priv); void
> > +intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv); bool
> > +intel_lpe_audio_detect(struct drm_i915_private *dev_priv);
> > +
> >  /* intel_i2c.c */
> >  extern int intel_setup_gmbus(struct drm_device *dev);  extern void
> > intel_teardown_gmbus(struct drm_device *dev); diff --git
> > a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 07ca71c..6c98b28 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1894,6 +1894,15 @@ static irqreturn_t valleyview_irq_handler(int irq,
> void *arg)
> >  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> >
> >  		/*
> > +		 * LPE audio interrupts are only enabled on Baytrail and
> > +		 * CherryView platforms without HDaudio
> > +		 */
> > +		if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> > +			   I915_LPE_PIPE_B_INTERRUPT |
> > +			   I915_LPE_PIPE_C_INTERRUPT))
> 
> No pipe C on vlv.
> 

OK

> > +			intel_lpe_audio_irq_handler(dev_priv);
> > +
> > +		/*
> >  		 * VLV_IIR is single buffered, and reflects the level
> >  		 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
> >  		 */
> > @@ -1974,6 +1983,15 @@ static irqreturn_t cherryview_irq_handler(int irq,
> void *arg)
> >  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> >
> >  		/*
> > +		 * LPE audio interrupts are only enabled on Baytrail and
> > +		 * CherryView platforms without HDaudio
> > +		 */
> > +		if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> > +			   I915_LPE_PIPE_B_INTERRUPT |
> > +			   I915_LPE_PIPE_C_INTERRUPT))
> > +			intel_lpe_audio_irq_handler(dev_priv);
> > +
> > +		/*
> >  		 * VLV_IIR is single buffered, and reflects the level
> >  		 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
> >  		 */
> > @@ -2930,6 +2948,17 @@ static void vlv_display_irq_postinstall(struct
> > drm_i915_private *dev_priv)
> >
> >  	WARN_ON(dev_priv->irq_mask != ~0);
> >
> > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> 
> This is vlv/chv specific code, so no need to check for it.
> 

OK

> > +		/* add interrupt masks unconditially here, the actual unmask
> > +		 * will take place only if the LPE_AUDIO mode is detected
> > +		 */
> > +		u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> > +			I915_LPE_PIPE_B_INTERRUPT |
> > +			I915_LPE_PIPE_C_INTERRUPT);
> > +
> > +		enable_mask |= val;
> > +	}
> > +
> >  	dev_priv->irq_mask = ~enable_mask;
> >
> >  	GEN5_IRQ_INIT(VLV_, dev_priv->irq_mask, enable_mask); diff --git
> > a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index fcad8fa..6b16ed0 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2395,6 +2395,9 @@ enum skl_disp_power_wells {
> >  #define I915_ASLE_INTERRUPT				(1<<0)
> >  #define I915_BSD_USER_INTERRUPT
> 	(1<<25)
> >
> > +#define I915_HDMI_LPE_AUDIO_BASE	(VLV_DISPLAY_BASE +
> 0x65000)
> > +#define I915_HDMI_LPE_AUDIO_SIZE	0x1000
> > +
> >  #define GEN6_BSD_RNCID			_MMIO(0x12198)
> >
> >  #define GEN7_FF_THREAD_MODE		_MMIO(0x20a0)
> > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > new file mode 100644
> > index 0000000..5335fc6
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > @@ -0,0 +1,369 @@
> > +/*
> > + * Copyright © 2016 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > +obtaining a
> > + * copy of this software and associated documentation files (the
> > +"Software"),
> > + * to deal in the Software without restriction, including without
> > +limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > +sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
> > +the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including
> > +the next
> > + * paragraph) shall be included in all copies or substantial portions
> > +of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> > +EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > +MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN
> NO EVENT
> > +SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES
> > +OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> OTHERWISE,
> > +ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> OR
> > +OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + *    Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > + *    Jerome Anand <jerome.anand@intel.com>
> > + *    based on VED patches
> > + *
> > + */
> > +
> > +/**
> > + * DOC: LPE Audio integration for HDMI or DP playback
> > + *
> > + * Motivation:
> > + * Atom platforms (e.g. valleyview and cherryTrail) integrates a
> > +DMA-based
> > + * interface as an alternative to the traditional HDaudio path. While
> > +this
> > + * mode is unrelated to the LPE aka SST audio engine, the
> > +documentation refers
> > + * to this mode as LPE so we keep this notation for the sake of
> consistency.
> > + *
> > + * The interface is handled by a separate standalone driver
> > +maintained in the
> > + * ALSA subsystem for simplicity. To minimize the interaction between
> > +the two
> > + * subsystems, a bridge is setup between the hdmi-lpe-audio and i915:
> > + * 1. Create a platform device to share MMIO/IRQ resources
> > + * 2. Make the platform device child of i915 device for runtime PM.
> > + * 3. Create IRQ chip to forward the LPE audio irqs.
> > + * the hdmi-lpe-audio driver probes the lpe audio device and creates
> > +a new
> > + * sound card
> > + *
> > + * Threats:
> > + * Due to the restriction in Linux platform device model, user need
> > +manually
> > + * uninstall the hdmi-lpe-audio driver before uninstalling i915
> > +module,
> > + * otherwise we might run into use-after-free issues after i915
> > +removes the
> > + * platform device: even though hdmi-lpe-audio driver is released,
> > +the modules
> > + * is still in "installed" status.
> > + *
> > + * Implementation:
> > + * The MMIO/REG platform resources are created according to the
> > +registers
> > + * specification.
> > + * When forwarding LPE audio irqs, the flow control handler selection
> > +depends
> > + * on the platform, for example on valleyview handle_simple_irq is
> enough.
> > + *
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/device.h>
> > +#include <linux/pci.h>
> > +
> > +#include "i915_drv.h"
> > +#include <linux/delay.h>
> > +#include <drm/intel_lpe_audio.h>
> > +
> > +static struct platform_device*
> > +lpe_audio_platdev_create(struct drm_i915_private *dev_priv) {
> > +	struct drm_device *dev = &dev_priv->drm;
> > +	int ret;
> > +	struct resource rsc[2] = {};
> > +	struct platform_device *platdev;
> > +	u64 *dma_mask;
> > +	struct intel_hdmi_lpe_audio_pdata *pdata;
> > +
> > +	if (dev_priv->lpe_audio.irq < 0)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	platdev = platform_device_alloc("hdmi-lpe-audio", -1);
> > +	if (!platdev) {
> > +		ret = -ENOMEM;
> > +		DRM_ERROR("Failed to allocate LPE audio platform
> device\n");
> > +		goto err;
> > +	}
> > +
> > +	/* to work-around check_addr in nommu_map_sg() */
> > +	dma_mask = kmalloc(sizeof(*platdev->dev.dma_mask),
> GFP_KERNEL);
> > +	if (!dma_mask) {
> > +		ret = -ENOMEM;
> > +		DRM_ERROR("Failed to allocate dma_mask\n");
> > +		goto err_put_dev;
> > +	}
> > +	*dma_mask = DMA_BIT_MASK(32);
> > +	platdev->dev.dma_mask = dma_mask;
> > +	platdev->dev.coherent_dma_mask = *dma_mask;
> > +
> > +	rsc[0].start    = rsc[0].end = dev_priv->lpe_audio.irq;
> > +	rsc[0].flags    = IORESOURCE_IRQ;
> > +	rsc[0].name     = "hdmi-lpe-audio-irq";
> > +
> > +	rsc[1].start    = pci_resource_start(dev->pdev, 0) +
> > +		I915_HDMI_LPE_AUDIO_BASE;
> > +	rsc[1].end      = pci_resource_start(dev->pdev, 0) +
> > +		I915_HDMI_LPE_AUDIO_BASE +
> I915_HDMI_LPE_AUDIO_SIZE - 1;
> > +	rsc[1].flags    = IORESOURCE_MEM;
> > +	rsc[1].name     = "hdmi-lpe-audio-mmio";
> > +
> > +	ret = platform_device_add_resources(platdev, rsc, 2);
> > +	if (ret) {
> > +		DRM_ERROR("Failed to add resource for platform device:
> %d\n",
> > +			ret);
> > +		goto err_put_dev;
> > +	}
> > +
> > +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > +
> > +	if (pdata == NULL) {
> > +		ret = -ENOMEM;
> > +		goto err_put_dev;
> > +	}
> > +	platdev->dev.platform_data = pdata;
> > +	spin_lock_init(&pdata->lpe_audio_slock);
> > +
> > +	/* for LPE audio driver's runtime-PM */
> > +	platdev->dev.parent = dev->dev;
> > +	ret = platform_device_add(platdev);
> > +	if (ret) {
> > +		DRM_ERROR("Failed to add LPE audio platform device:
> %d\n",
> > +			ret);
> > +		goto err_put_dev;
> > +	}
> > +
> > +	return platdev;
> > +err_put_dev:
> > +	platform_device_put(platdev);
> > +	kfree(dma_mask);
> > +err:
> > +	return ERR_PTR(ret);
> > +}
> > +
> > +static void lpe_audio_platdev_destroy(struct drm_i915_private
> > +*dev_priv) {
> > +	if (!HAS_LPE_AUDIO(dev_priv))
> > +		return;
> > +	platform_device_unregister(dev_priv->lpe_audio.platdev);
> > +	kfree(dev_priv->lpe_audio.platdev->dev.dma_mask);
> > +}
> > +
> > +static void lpe_audio_irq_unmask(struct irq_data *d) {
> > +	struct drm_device *dev = d->chip_data;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	unsigned long irqflags;
> > +	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> > +		I915_LPE_PIPE_B_INTERRUPT |
> > +		I915_LPE_PIPE_C_INTERRUPT);
> 
> No pipe C on vlv.
> 

OK

> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > +	/*
> > +	 * VLV_IER is already set in the vlv_display_postinstall(),
> > +	 * we only change VLV_IIR and VLV_IMR
> > +	 */
> > +	dev_priv->irq_mask &= ~val;
> > +	I915_WRITE(VLV_IIR, val);
> > +	I915_WRITE(VLV_IIR, val);
> > +	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +	POSTING_READ(VLV_IMR);
> > +
> > +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); }
> > +
> > +static void lpe_audio_irq_mask(struct irq_data *d) {
> > +	struct drm_device *dev = d->chip_data;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	unsigned long irqflags;
> > +	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> > +		I915_LPE_PIPE_B_INTERRUPT |
> > +		I915_LPE_PIPE_C_INTERRUPT);
> 
> ditto
> 

OK

> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > +	/*
> > +	 * VLV_IER is already set in the vlv_display_postinstall(),
> > +	 * we only change VLV_IIR and VLV_IMR
> > +	 */
> > +	dev_priv->irq_mask |= val;
> > +	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +	I915_WRITE(VLV_IIR, val);
> > +	I915_WRITE(VLV_IIR, val);
> > +	POSTING_READ(VLV_IIR);
> > +
> > +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); }
> > +
> > +static struct irq_chip lpe_audio_irqchip = {
> > +	.name = "hdmi_lpe_audio_irqchip",
> > +	.irq_mask = lpe_audio_irq_mask,
> > +	.irq_unmask = lpe_audio_irq_unmask,
> > +};
> > +
> > +static int lpe_audio_irq_init(struct drm_i915_private *dev_priv) {
> > +	int irq = dev_priv->lpe_audio.irq;
> > +
> > +	WARN_ON(!intel_irqs_enabled(dev_priv));
> > +	irq_set_chip_and_handler_name(irq,
> > +				&lpe_audio_irqchip,
> > +				handle_simple_irq,
> > +				"hdmi_lpe_audio_irq_handler");
> > +
> > +	return irq_set_chip_data(irq, &dev_priv->drm);
> 
> Just pass dev_priv as the chip data.
> 

OK

> > +}
> > +
> > +/**
> > + * intel_lpe_audio_irq_handler() - forwards the LPE audio irq
> > + * @dev_priv: the i915 drm device private data
> > + *
> > + * the LPE Audio irq is forwarded to the irq handler registered by
> > +LPE audio
> > + * driver.
> > + */
> > +void intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv) {
> > +	int ret;
> > +
> > +	ret = generic_handle_irq(dev_priv->lpe_audio.irq);
> > +	if (ret)
> > +		DRM_ERROR_RATELIMITED("error handling LPE audio irq:
> %d\n",
> > +				ret);
> > +}
> > +
> > +/**
> > + * intel_lpe_audio_detect() - check & setup lpe audio if present
> > + * @dev_priv: the i915 drm device private data
> > + *
> > + * Detect if lpe audio is present
> > + *
> > + * Return: true if lpe audio present else Return = false  */ bool
> > +intel_lpe_audio_detect(struct drm_i915_private *dev_priv) {
> > +	int lpe_present = false;
> > +
> > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> 
> Could avoid indenting the whole thig by reversing this test and doing an early
> return.
> 

Will prefer the existing method

> > +		static const struct pci_device_id atom_hdaudio_ids[] = {
> > +			/* Baytrail */
> > +			{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x0f04)},
> > +			/* Braswell */
> > +			{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x2284)},
> > +			{}
> > +		};
> 
> IIRC I asked if we could use a HDA class match instead. But I forget what the
> answer was. No presumably?
> 

The previous reply was " we are not aware of any such class that exists at present"

> > +
> > +		if (!pci_dev_present(atom_hdaudio_ids)) {
> > +			DRM_INFO("%s%s\n", "HDaudio controller not
> detected,",
> > +				"using LPE audio instead\n");
> > +			lpe_present = true;
> > +		}
> > +	}
> > +	return lpe_present;
> > +}
> > +
> > +/**
> > + * intel_lpe_audio_setup() - setup the bridge between HDMI LPE Audio
> > + * driver and i915
> > + * @dev_priv: the i915 drm device private data
> > + *
> > + * set up the minimum required resources for the bridge: irq chip,
> > + * platform resource and platform device. i915 device is set as
> > +parent
> > + * of the new platform device.
> > + *
> > + * Return: 0 if successful. non-zero if allocation/initialization
> > +fails  */ int intel_lpe_audio_setup(struct drm_i915_private
> > +*dev_priv) {
> > +	int ret;
> > +
> > +	dev_priv->lpe_audio.irq = irq_alloc_desc(0);
> > +	if (dev_priv->lpe_audio.irq < 0) {
> > +		DRM_ERROR("Failed to allocate IRQ desc: %d\n",
> > +			dev_priv->lpe_audio.irq);
> > +		ret = dev_priv->lpe_audio.irq;
> > +		goto err;
> > +	}
> > +
> > +	DRM_DEBUG("irq = %d\n", dev_priv->lpe_audio.irq);
> > +
> > +	ret = lpe_audio_irq_init(dev_priv);
> > +
> > +	if (ret) {
> > +		DRM_ERROR("Failed to initialize irqchip for lpe audio: %d\n",
> > +			ret);
> > +		goto err_free_irq;
> > +	}
> > +
> > +	dev_priv->lpe_audio.platdev = lpe_audio_platdev_create(dev_priv);
> > +
> > +	if (IS_ERR(dev_priv->lpe_audio.platdev)) {
> > +		ret = PTR_ERR(dev_priv->lpe_audio.platdev);
> > +		DRM_ERROR("Failed to create lpe audio platform device:
> %d\n",
> > +			ret);
> > +		goto err_free_irq;
> > +	}
> > +
> > +	return 0;
> > +err_free_irq:
> > +	irq_free_desc(dev_priv->lpe_audio.irq);
> > +err:
> > +	dev_priv->lpe_audio.irq = -1;
> > +	dev_priv->lpe_audio.platdev = NULL;
> > +	return ret;
> > +}
> > +
> > +
> > +/**
> > + * intel_lpe_audio_init() - detect and setup the bridge between HDMI
> > +LPE Audio
> > + * driver and i915
> > + * @dev_priv: the i915 drm device private data
> > + *
> > + * Return: 0 if successful. non-zero if detection or
> > + * llocation/initialization fails
> > + */
> > +int intel_lpe_audio_init(struct drm_i915_private *dev_priv) {
> > +	int ret = -ENODEV;
> > +
> > +	if (intel_lpe_audio_detect(dev_priv)) {
> > +		ret = intel_lpe_audio_setup(dev_priv);
> > +		if (ret < 0)
> > +			DRM_ERROR("failed to setup LPE Audio bridge\n");
> > +	}
> > +	return ret;
> > +}
> > +
> > +/**
> > + * intel_lpe_audio_teardown() - destroy the bridge between HDMI LPE
> > + * audio driver and i915
> > + * @dev_priv: the i915 drm device private data
> > + *
> > + * release all the resources for LPE audio <-> i915 bridge.
> > + */
> > +void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv) {
> > +	unsigned long irqflags;
> > +	struct irq_desc *desc;
> > +
> > +	desc = (dev_priv->lpe_audio.irq >= 0) ?
> > +		irq_to_desc(dev_priv->lpe_audio.irq) : NULL;
> 
> Why not just "if (lpe not initialized) return;" ?
> 

Need to perform cleaning in spite of it

> > +
> > +	/**
> > +	 * mask LPE audio irq before destroying
> > +	 */
> > +	if (desc)
> > +		lpe_audio_irq_mask(&desc->irq_data);
> 
> This seems racy. Should unregister the platdev first I think,
> 

OK

> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > +	lpe_audio_platdev_destroy(dev_priv);
> > +
> > +	if (desc)
> > +		irq_free_desc(dev_priv->lpe_audio.irq);
> > +
> > +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); }
> > diff --git a/include/drm/intel_lpe_audio.h
> > b/include/drm/intel_lpe_audio.h new file mode 100644 index
> > 0000000..a64c449
> > --- /dev/null
> > +++ b/include/drm/intel_lpe_audio.h
> > @@ -0,0 +1,45 @@
> > +/*
> > + * Copyright © 2016 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > +obtaining a
> > + * copy of this software and associated documentation files (the
> > +"Software"),
> > + * to deal in the Software without restriction, including without
> > +limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > +sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
> > +the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including
> > +the next
> > + * paragraph) shall be included in all copies or substantial portions
> > +of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> > +EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > +MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN
> NO EVENT
> > +SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES
> > +OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> OTHERWISE,
> > +ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> OR
> > +OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef _INTEL_LPE_AUDIO_H_
> > +#define _INTEL_LPE_AUDIO_H_
> > +
> > +#include <linux/types.h>
> > +
> > +#define HDMI_MAX_ELD_BYTES	128
> > +
> > +struct intel_hdmi_lpe_audio_eld {
> > +	int port_id;
> > +	unsigned char eld_data[HDMI_MAX_ELD_BYTES]; };
> > +
> > +struct intel_hdmi_lpe_audio_pdata {
> > +	bool notify_pending;
> > +	int tmds_clock_speed;
> > +	bool hdmi_connected;
> > +	struct intel_hdmi_lpe_audio_eld eld;
> > +	void (*notify_audio_lpe)(void *audio_ptr);
> > +	spinlock_t lpe_audio_slock;
> > +};
> > +
> > +#endif /* _I915_LPE_AUDIO_H_ */
> > --
> > 2.9.3
> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI LPE audio driver

[Ville Syrjälä]

On Fri, Nov 25, 2016 at 05:42:38AM +0000, Anand, Jerome wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Thursday, November 24, 2016 7:02 PM
> > To: Anand, Jerome <jerome.anand@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org;
> > broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com;
> > Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> > Subject: Re: [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI LPE audio
> > driver
> > 
> > On Fri, Nov 25, 2016 at 05:25:42AM +0530, Jerome Anand wrote:
> > > Enable support for HDMI LPE audio mode on Baytrail and Cherrytrail
> > > when HDaudio controller is not detected
> > >
> > > Setup minimum required resources during i915_driver_load:
> > > 1. Create a platform device to share MMIO/IRQ resources 2. Make the
> > > platform device child of i915 device for runtime PM.
> > > 3. Create IRQ chip to forward HDMI LPE audio irqs.
> > >
> > > HDMI LPE audio driver (a standalone sound driver) probes the LPE audio
> > > device and creates a new sound card.
> > >
> > > Signed-off-by: Pierre-Louis Bossart
> > > <pierre-louis.bossart@linux.intel.com>
> > > Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> > > ---
> > >  Documentation/gpu/i915.rst             |   9 +
> > >  drivers/gpu/drm/i915/Makefile          |   3 +
> > >  drivers/gpu/drm/i915/i915_drv.c        |   8 +-
> > >  drivers/gpu/drm/i915/i915_drv.h        |  15 ++
> > >  drivers/gpu/drm/i915/i915_irq.c        |  29 +++
> > >  drivers/gpu/drm/i915/i915_reg.h        |   3 +
> > >  drivers/gpu/drm/i915/intel_lpe_audio.c | 369
> > +++++++++++++++++++++++++++++++++
> > >  include/drm/intel_lpe_audio.h          |  45 ++++
> > >  8 files changed, 479 insertions(+), 2 deletions(-)  create mode
> > > 100644 drivers/gpu/drm/i915/intel_lpe_audio.c
> > >  create mode 100644 include/drm/intel_lpe_audio.h
> > >
> > > diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> > > index 117d2ab..a671eee 100644
> > > --- a/Documentation/gpu/i915.rst
> > > +++ b/Documentation/gpu/i915.rst
> > > @@ -213,6 +213,15 @@ Video BIOS Table (VBT)  .. kernel-doc::
> > > drivers/gpu/drm/i915/intel_vbt_defs.h
> > >     :internal:
> > >
> > > +intel hdmi lpe audio support
> > > +----------------------------
> > > +
> > > +.. kernel-doc:: drivers/gpu/drm/i915/intel_lpe_audio.c
> > > +   :doc:  LPE Audio integration for HDMI or DP playback
> > > +
> > > +.. kernel-doc:: drivers/gpu/drm/i915/intel_lpe_audio.c
> > > +   :internal:
> > > +
> > >  Memory Management and Command Submission
> > > ========================================
> > >
> > > diff --git a/drivers/gpu/drm/i915/Makefile
> > > b/drivers/gpu/drm/i915/Makefile index 580602d..3a2369c 100644
> > > --- a/drivers/gpu/drm/i915/Makefile
> > > +++ b/drivers/gpu/drm/i915/Makefile
> > > @@ -126,6 +126,9 @@ i915-y += intel_gvt.o  include $(src)/gvt/Makefile
> > > endif
> > >
> > > +# LPE Audio for VLV and CHT
> > > +i915-y += intel_lpe_audio.o
> > > +
> > >  obj-$(CONFIG_DRM_I915) += i915.o
> > >
> > >  CFLAGS_i915_trace_points.o := -I$(src) diff --git
> > > a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index b893e67..93811ed 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1144,7 +1144,8 @@ static void i915_driver_register(struct
> > drm_i915_private *dev_priv)
> > >  	if (IS_GEN5(dev_priv))
> > >  		intel_gpu_ips_init(dev_priv);
> > >
> > > -	i915_audio_component_init(dev_priv);
> > > +	if (intel_lpe_audio_init(dev_priv) < 0)
> > > +		i915_audio_component_init(dev_priv);
> > >
> > >  	/*
> > >  	 * Some ports require correctly set-up hpd registers for detection
> > > to @@ -1162,7 +1163,10 @@ static void i915_driver_register(struct
> > drm_i915_private *dev_priv)
> > >   */
> > >  static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> > > {
> > > -	i915_audio_component_cleanup(dev_priv);
> > > +	if (HAS_LPE_AUDIO(dev_priv))
> > > +		intel_lpe_audio_teardown(dev_priv);
> > > +	else
> > > +		i915_audio_component_cleanup(dev_priv);
> > >
> > >  	intel_gpu_ips_teardown();
> > >  	acpi_video_unregister();
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h index 970e50b..2a79048 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2284,6 +2284,12 @@ struct drm_i915_private {
> > >  	/* Used to save the pipe-to-encoder mapping for audio */
> > >  	struct intel_encoder *av_enc_map[I915_MAX_PIPES];
> > >
> > > +	/* necessary resource sharing with HDMI LPE audio driver. */
> > > +	struct {
> > > +		struct platform_device *platdev;
> > > +		int	irq;
> > > +	} lpe_audio;
> > > +
> > >  	/*
> > >  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your
> > patch
> > >  	 * will be rejected. Instead look for a better place.
> > > @@ -2773,6 +2779,8 @@ intel_info(const struct drm_i915_private
> > > *dev_priv)
> > >
> > >  #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
> > >
> > > +#define HAS_LPE_AUDIO(dev_priv) ((dev_priv)->lpe_audio.platdev !=
> > > +NULL)
> > > +
> > >  #define INTEL_PCH_DEVICE_ID_MASK		0xff00
> > >  #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
> > >  #define INTEL_PCH_CPT_DEVICE_ID_TYPE		0x1c00
> > > @@ -3547,6 +3555,13 @@ extern int i915_restore_state(struct drm_device
> > > *dev);  void i915_setup_sysfs(struct drm_i915_private *dev_priv);
> > > void i915_teardown_sysfs(struct drm_i915_private *dev_priv);
> > >
> > > +/* i915_lpe_audio.c */
> > > +int  intel_lpe_audio_init(struct drm_i915_private *dev_priv); int
> > > +intel_lpe_audio_setup(struct drm_i915_private *dev_priv); void
> > > +intel_lpe_audio_teardown(struct drm_i915_private *dev_priv); void
> > > +intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv); bool
> > > +intel_lpe_audio_detect(struct drm_i915_private *dev_priv);
> > > +
> > >  /* intel_i2c.c */
> > >  extern int intel_setup_gmbus(struct drm_device *dev);  extern void
> > > intel_teardown_gmbus(struct drm_device *dev); diff --git
> > > a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 07ca71c..6c98b28 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -1894,6 +1894,15 @@ static irqreturn_t valleyview_irq_handler(int irq,
> > void *arg)
> > >  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> > >
> > >  		/*
> > > +		 * LPE audio interrupts are only enabled on Baytrail and
> > > +		 * CherryView platforms without HDaudio
> > > +		 */
> > > +		if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> > > +			   I915_LPE_PIPE_B_INTERRUPT |
> > > +			   I915_LPE_PIPE_C_INTERRUPT))
> > 
> > No pipe C on vlv.
> > 
> 
> OK
> 
> > > +			intel_lpe_audio_irq_handler(dev_priv);
> > > +
> > > +		/*
> > >  		 * VLV_IIR is single buffered, and reflects the level
> > >  		 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
> > >  		 */
> > > @@ -1974,6 +1983,15 @@ static irqreturn_t cherryview_irq_handler(int irq,
> > void *arg)
> > >  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> > >
> > >  		/*
> > > +		 * LPE audio interrupts are only enabled on Baytrail and
> > > +		 * CherryView platforms without HDaudio
> > > +		 */
> > > +		if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> > > +			   I915_LPE_PIPE_B_INTERRUPT |
> > > +			   I915_LPE_PIPE_C_INTERRUPT))
> > > +			intel_lpe_audio_irq_handler(dev_priv);
> > > +
> > > +		/*
> > >  		 * VLV_IIR is single buffered, and reflects the level
> > >  		 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
> > >  		 */
> > > @@ -2930,6 +2948,17 @@ static void vlv_display_irq_postinstall(struct
> > > drm_i915_private *dev_priv)
> > >
> > >  	WARN_ON(dev_priv->irq_mask != ~0);
> > >
> > > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > 
> > This is vlv/chv specific code, so no need to check for it.
> > 
> 
> OK
> 
> > > +		/* add interrupt masks unconditially here, the actual unmask
> > > +		 * will take place only if the LPE_AUDIO mode is detected
> > > +		 */
> > > +		u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> > > +			I915_LPE_PIPE_B_INTERRUPT |
> > > +			I915_LPE_PIPE_C_INTERRUPT);
> > > +
> > > +		enable_mask |= val;
> > > +	}
> > > +
> > >  	dev_priv->irq_mask = ~enable_mask;
> > >
> > >  	GEN5_IRQ_INIT(VLV_, dev_priv->irq_mask, enable_mask); diff --git
> > > a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index fcad8fa..6b16ed0 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -2395,6 +2395,9 @@ enum skl_disp_power_wells {
> > >  #define I915_ASLE_INTERRUPT				(1<<0)
> > >  #define I915_BSD_USER_INTERRUPT
> > 	(1<<25)
> > >
> > > +#define I915_HDMI_LPE_AUDIO_BASE	(VLV_DISPLAY_BASE +
> > 0x65000)
> > > +#define I915_HDMI_LPE_AUDIO_SIZE	0x1000
> > > +
> > >  #define GEN6_BSD_RNCID			_MMIO(0x12198)
> > >
> > >  #define GEN7_FF_THREAD_MODE		_MMIO(0x20a0)
> > > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > new file mode 100644
> > > index 0000000..5335fc6
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > @@ -0,0 +1,369 @@
> > > +/*
> > > + * Copyright © 2016 Intel Corporation
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person
> > > +obtaining a
> > > + * copy of this software and associated documentation files (the
> > > +"Software"),
> > > + * to deal in the Software without restriction, including without
> > > +limitation
> > > + * the rights to use, copy, modify, merge, publish, distribute,
> > > +sublicense,
> > > + * and/or sell copies of the Software, and to permit persons to whom
> > > +the
> > > + * Software is furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice (including
> > > +the next
> > > + * paragraph) shall be included in all copies or substantial portions
> > > +of the
> > > + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> > KIND,
> > > +EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > +MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN
> > NO EVENT
> > > +SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES
> > > +OR OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> > OTHERWISE,
> > > +ARISING
> > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> > OR
> > > +OTHER DEALINGS
> > > + * IN THE SOFTWARE.
> > > + *
> > > + * Authors:
> > > + *    Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > + *    Jerome Anand <jerome.anand@intel.com>
> > > + *    based on VED patches
> > > + *
> > > + */
> > > +
> > > +/**
> > > + * DOC: LPE Audio integration for HDMI or DP playback
> > > + *
> > > + * Motivation:
> > > + * Atom platforms (e.g. valleyview and cherryTrail) integrates a
> > > +DMA-based
> > > + * interface as an alternative to the traditional HDaudio path. While
> > > +this
> > > + * mode is unrelated to the LPE aka SST audio engine, the
> > > +documentation refers
> > > + * to this mode as LPE so we keep this notation for the sake of
> > consistency.
> > > + *
> > > + * The interface is handled by a separate standalone driver
> > > +maintained in the
> > > + * ALSA subsystem for simplicity. To minimize the interaction between
> > > +the two
> > > + * subsystems, a bridge is setup between the hdmi-lpe-audio and i915:
> > > + * 1. Create a platform device to share MMIO/IRQ resources
> > > + * 2. Make the platform device child of i915 device for runtime PM.
> > > + * 3. Create IRQ chip to forward the LPE audio irqs.
> > > + * the hdmi-lpe-audio driver probes the lpe audio device and creates
> > > +a new
> > > + * sound card
> > > + *
> > > + * Threats:
> > > + * Due to the restriction in Linux platform device model, user need
> > > +manually
> > > + * uninstall the hdmi-lpe-audio driver before uninstalling i915
> > > +module,
> > > + * otherwise we might run into use-after-free issues after i915
> > > +removes the
> > > + * platform device: even though hdmi-lpe-audio driver is released,
> > > +the modules
> > > + * is still in "installed" status.
> > > + *
> > > + * Implementation:
> > > + * The MMIO/REG platform resources are created according to the
> > > +registers
> > > + * specification.
> > > + * When forwarding LPE audio irqs, the flow control handler selection
> > > +depends
> > > + * on the platform, for example on valleyview handle_simple_irq is
> > enough.
> > > + *
> > > + */
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/device.h>
> > > +#include <linux/pci.h>
> > > +
> > > +#include "i915_drv.h"
> > > +#include <linux/delay.h>
> > > +#include <drm/intel_lpe_audio.h>
> > > +
> > > +static struct platform_device*
> > > +lpe_audio_platdev_create(struct drm_i915_private *dev_priv) {
> > > +	struct drm_device *dev = &dev_priv->drm;
> > > +	int ret;
> > > +	struct resource rsc[2] = {};
> > > +	struct platform_device *platdev;
> > > +	u64 *dma_mask;
> > > +	struct intel_hdmi_lpe_audio_pdata *pdata;
> > > +
> > > +	if (dev_priv->lpe_audio.irq < 0)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	platdev = platform_device_alloc("hdmi-lpe-audio", -1);
> > > +	if (!platdev) {
> > > +		ret = -ENOMEM;
> > > +		DRM_ERROR("Failed to allocate LPE audio platform
> > device\n");
> > > +		goto err;
> > > +	}
> > > +
> > > +	/* to work-around check_addr in nommu_map_sg() */
> > > +	dma_mask = kmalloc(sizeof(*platdev->dev.dma_mask),
> > GFP_KERNEL);
> > > +	if (!dma_mask) {
> > > +		ret = -ENOMEM;
> > > +		DRM_ERROR("Failed to allocate dma_mask\n");
> > > +		goto err_put_dev;
> > > +	}
> > > +	*dma_mask = DMA_BIT_MASK(32);
> > > +	platdev->dev.dma_mask = dma_mask;
> > > +	platdev->dev.coherent_dma_mask = *dma_mask;
> > > +
> > > +	rsc[0].start    = rsc[0].end = dev_priv->lpe_audio.irq;
> > > +	rsc[0].flags    = IORESOURCE_IRQ;
> > > +	rsc[0].name     = "hdmi-lpe-audio-irq";
> > > +
> > > +	rsc[1].start    = pci_resource_start(dev->pdev, 0) +
> > > +		I915_HDMI_LPE_AUDIO_BASE;
> > > +	rsc[1].end      = pci_resource_start(dev->pdev, 0) +
> > > +		I915_HDMI_LPE_AUDIO_BASE +
> > I915_HDMI_LPE_AUDIO_SIZE - 1;
> > > +	rsc[1].flags    = IORESOURCE_MEM;
> > > +	rsc[1].name     = "hdmi-lpe-audio-mmio";
> > > +
> > > +	ret = platform_device_add_resources(platdev, rsc, 2);
> > > +	if (ret) {
> > > +		DRM_ERROR("Failed to add resource for platform device:
> > %d\n",
> > > +			ret);
> > > +		goto err_put_dev;
> > > +	}
> > > +
> > > +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> > > +
> > > +	if (pdata == NULL) {
> > > +		ret = -ENOMEM;
> > > +		goto err_put_dev;
> > > +	}
> > > +	platdev->dev.platform_data = pdata;
> > > +	spin_lock_init(&pdata->lpe_audio_slock);
> > > +
> > > +	/* for LPE audio driver's runtime-PM */
> > > +	platdev->dev.parent = dev->dev;
> > > +	ret = platform_device_add(platdev);
> > > +	if (ret) {
> > > +		DRM_ERROR("Failed to add LPE audio platform device:
> > %d\n",
> > > +			ret);
> > > +		goto err_put_dev;
> > > +	}
> > > +
> > > +	return platdev;
> > > +err_put_dev:
> > > +	platform_device_put(platdev);
> > > +	kfree(dma_mask);
> > > +err:
> > > +	return ERR_PTR(ret);
> > > +}
> > > +
> > > +static void lpe_audio_platdev_destroy(struct drm_i915_private
> > > +*dev_priv) {
> > > +	if (!HAS_LPE_AUDIO(dev_priv))
> > > +		return;
> > > +	platform_device_unregister(dev_priv->lpe_audio.platdev);
> > > +	kfree(dev_priv->lpe_audio.platdev->dev.dma_mask);
> > > +}
> > > +
> > > +static void lpe_audio_irq_unmask(struct irq_data *d) {
> > > +	struct drm_device *dev = d->chip_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > > +	unsigned long irqflags;
> > > +	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> > > +		I915_LPE_PIPE_B_INTERRUPT |
> > > +		I915_LPE_PIPE_C_INTERRUPT);
> > 
> > No pipe C on vlv.
> > 
> 
> OK
> 
> > > +
> > > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > > +
> > > +	/*
> > > +	 * VLV_IER is already set in the vlv_display_postinstall(),
> > > +	 * we only change VLV_IIR and VLV_IMR
> > > +	 */
> > > +	dev_priv->irq_mask &= ~val;
> > > +	I915_WRITE(VLV_IIR, val);
> > > +	I915_WRITE(VLV_IIR, val);
> > > +	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > > +	POSTING_READ(VLV_IMR);
> > > +
> > > +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); }
> > > +
> > > +static void lpe_audio_irq_mask(struct irq_data *d) {
> > > +	struct drm_device *dev = d->chip_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > > +	unsigned long irqflags;
> > > +	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> > > +		I915_LPE_PIPE_B_INTERRUPT |
> > > +		I915_LPE_PIPE_C_INTERRUPT);
> > 
> > ditto
> > 
> 
> OK
> 
> > > +
> > > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > > +
> > > +	/*
> > > +	 * VLV_IER is already set in the vlv_display_postinstall(),
> > > +	 * we only change VLV_IIR and VLV_IMR
> > > +	 */
> > > +	dev_priv->irq_mask |= val;
> > > +	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > > +	I915_WRITE(VLV_IIR, val);
> > > +	I915_WRITE(VLV_IIR, val);
> > > +	POSTING_READ(VLV_IIR);
> > > +
> > > +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); }
> > > +
> > > +static struct irq_chip lpe_audio_irqchip = {
> > > +	.name = "hdmi_lpe_audio_irqchip",
> > > +	.irq_mask = lpe_audio_irq_mask,
> > > +	.irq_unmask = lpe_audio_irq_unmask,
> > > +};
> > > +
> > > +static int lpe_audio_irq_init(struct drm_i915_private *dev_priv) {
> > > +	int irq = dev_priv->lpe_audio.irq;
> > > +
> > > +	WARN_ON(!intel_irqs_enabled(dev_priv));
> > > +	irq_set_chip_and_handler_name(irq,
> > > +				&lpe_audio_irqchip,
> > > +				handle_simple_irq,
> > > +				"hdmi_lpe_audio_irq_handler");
> > > +
> > > +	return irq_set_chip_data(irq, &dev_priv->drm);
> > 
> > Just pass dev_priv as the chip data.
> > 
> 
> OK
> 
> > > +}
> > > +
> > > +/**
> > > + * intel_lpe_audio_irq_handler() - forwards the LPE audio irq
> > > + * @dev_priv: the i915 drm device private data
> > > + *
> > > + * the LPE Audio irq is forwarded to the irq handler registered by
> > > +LPE audio
> > > + * driver.
> > > + */
> > > +void intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv) {
> > > +	int ret;
> > > +
> > > +	ret = generic_handle_irq(dev_priv->lpe_audio.irq);
> > > +	if (ret)
> > > +		DRM_ERROR_RATELIMITED("error handling LPE audio irq:
> > %d\n",
> > > +				ret);
> > > +}
> > > +
> > > +/**
> > > + * intel_lpe_audio_detect() - check & setup lpe audio if present
> > > + * @dev_priv: the i915 drm device private data
> > > + *
> > > + * Detect if lpe audio is present
> > > + *
> > > + * Return: true if lpe audio present else Return = false  */ bool
> > > +intel_lpe_audio_detect(struct drm_i915_private *dev_priv) {
> > > +	int lpe_present = false;
> > > +
> > > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > 
> > Could avoid indenting the whole thig by reversing this test and doing an early
> > return.
> > 
> 
> Will prefer the existing method
> 
> > > +		static const struct pci_device_id atom_hdaudio_ids[] = {
> > > +			/* Baytrail */
> > > +			{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x0f04)},
> > > +			/* Braswell */
> > > +			{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x2284)},
> > > +			{}
> > > +		};
> > 
> > IIRC I asked if we could use a HDA class match instead. But I forget what the
> > answer was. No presumably?
> > 
> 
> The previous reply was " we are not aware of any such class that exists at present"

Hmm. 0x0403 would be audio device. I guess we can't really use that
unless we combine it with the devfn (assuming the hda controller is
alwauys device 3. So yeah, specific device IDs seem better.

> 
> > > +
> > > +		if (!pci_dev_present(atom_hdaudio_ids)) {
> > > +			DRM_INFO("%s%s\n", "HDaudio controller not
> > detected,",
> > > +				"using LPE audio instead\n");
> > > +			lpe_present = true;
> > > +		}
> > > +	}
> > > +	return lpe_present;
> > > +}
> > > +
> > > +/**
> > > + * intel_lpe_audio_setup() - setup the bridge between HDMI LPE Audio
> > > + * driver and i915
> > > + * @dev_priv: the i915 drm device private data
> > > + *
> > > + * set up the minimum required resources for the bridge: irq chip,
> > > + * platform resource and platform device. i915 device is set as
> > > +parent
> > > + * of the new platform device.
> > > + *
> > > + * Return: 0 if successful. non-zero if allocation/initialization
> > > +fails  */ int intel_lpe_audio_setup(struct drm_i915_private
> > > +*dev_priv) {
> > > +	int ret;
> > > +
> > > +	dev_priv->lpe_audio.irq = irq_alloc_desc(0);
> > > +	if (dev_priv->lpe_audio.irq < 0) {
> > > +		DRM_ERROR("Failed to allocate IRQ desc: %d\n",
> > > +			dev_priv->lpe_audio.irq);
> > > +		ret = dev_priv->lpe_audio.irq;
> > > +		goto err;
> > > +	}
> > > +
> > > +	DRM_DEBUG("irq = %d\n", dev_priv->lpe_audio.irq);
> > > +
> > > +	ret = lpe_audio_irq_init(dev_priv);
> > > +
> > > +	if (ret) {
> > > +		DRM_ERROR("Failed to initialize irqchip for lpe audio: %d\n",
> > > +			ret);
> > > +		goto err_free_irq;
> > > +	}
> > > +
> > > +	dev_priv->lpe_audio.platdev = lpe_audio_platdev_create(dev_priv);
> > > +
> > > +	if (IS_ERR(dev_priv->lpe_audio.platdev)) {
> > > +		ret = PTR_ERR(dev_priv->lpe_audio.platdev);
> > > +		DRM_ERROR("Failed to create lpe audio platform device:
> > %d\n",
> > > +			ret);
> > > +		goto err_free_irq;
> > > +	}
> > > +
> > > +	return 0;
> > > +err_free_irq:
> > > +	irq_free_desc(dev_priv->lpe_audio.irq);
> > > +err:
> > > +	dev_priv->lpe_audio.irq = -1;
> > > +	dev_priv->lpe_audio.platdev = NULL;
> > > +	return ret;
> > > +}
> > > +
> > > +
> > > +/**
> > > + * intel_lpe_audio_init() - detect and setup the bridge between HDMI
> > > +LPE Audio
> > > + * driver and i915
> > > + * @dev_priv: the i915 drm device private data
> > > + *
> > > + * Return: 0 if successful. non-zero if detection or
> > > + * llocation/initialization fails
> > > + */
> > > +int intel_lpe_audio_init(struct drm_i915_private *dev_priv) {
> > > +	int ret = -ENODEV;
> > > +
> > > +	if (intel_lpe_audio_detect(dev_priv)) {
> > > +		ret = intel_lpe_audio_setup(dev_priv);
> > > +		if (ret < 0)
> > > +			DRM_ERROR("failed to setup LPE Audio bridge\n");
> > > +	}
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * intel_lpe_audio_teardown() - destroy the bridge between HDMI LPE
> > > + * audio driver and i915
> > > + * @dev_priv: the i915 drm device private data
> > > + *
> > > + * release all the resources for LPE audio <-> i915 bridge.
> > > + */
> > > +void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv) {
> > > +	unsigned long irqflags;
> > > +	struct irq_desc *desc;
> > > +
> > > +	desc = (dev_priv->lpe_audio.irq >= 0) ?
> > > +		irq_to_desc(dev_priv->lpe_audio.irq) : NULL;
> > 
> > Why not just "if (lpe not initialized) return;" ?
> > 
> 
> Need to perform cleaning in spite of it

Why? Partial initialization doesn't sounds good.

> 
> > > +
> > > +	/**
> > > +	 * mask LPE audio irq before destroying
> > > +	 */
> > > +	if (desc)
> > > +		lpe_audio_irq_mask(&desc->irq_data);
> > 
> > This seems racy. Should unregister the platdev first I think,
> > 
> 
> OK
> 
> > > +
> > > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > > +
> > > +	lpe_audio_platdev_destroy(dev_priv);
> > > +
> > > +	if (desc)
> > > +		irq_free_desc(dev_priv->lpe_audio.irq);
> > > +
> > > +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); }
> > > diff --git a/include/drm/intel_lpe_audio.h
> > > b/include/drm/intel_lpe_audio.h new file mode 100644 index
> > > 0000000..a64c449
> > > --- /dev/null
> > > +++ b/include/drm/intel_lpe_audio.h
> > > @@ -0,0 +1,45 @@
> > > +/*
> > > + * Copyright © 2016 Intel Corporation
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person
> > > +obtaining a
> > > + * copy of this software and associated documentation files (the
> > > +"Software"),
> > > + * to deal in the Software without restriction, including without
> > > +limitation
> > > + * the rights to use, copy, modify, merge, publish, distribute,
> > > +sublicense,
> > > + * and/or sell copies of the Software, and to permit persons to whom
> > > +the
> > > + * Software is furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice (including
> > > +the next
> > > + * paragraph) shall be included in all copies or substantial portions
> > > +of the
> > > + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> > KIND,
> > > +EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > +MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN
> > NO EVENT
> > > +SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES
> > > +OR OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> > OTHERWISE,
> > > +ARISING
> > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> > OR
> > > +OTHER DEALINGS
> > > + * IN THE SOFTWARE.
> > > + */
> > > +
> > > +#ifndef _INTEL_LPE_AUDIO_H_
> > > +#define _INTEL_LPE_AUDIO_H_
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +#define HDMI_MAX_ELD_BYTES	128
> > > +
> > > +struct intel_hdmi_lpe_audio_eld {
> > > +	int port_id;
> > > +	unsigned char eld_data[HDMI_MAX_ELD_BYTES]; };
> > > +
> > > +struct intel_hdmi_lpe_audio_pdata {
> > > +	bool notify_pending;
> > > +	int tmds_clock_speed;
> > > +	bool hdmi_connected;
> > > +	struct intel_hdmi_lpe_audio_eld eld;
> > > +	void (*notify_audio_lpe)(void *audio_ptr);
> > > +	spinlock_t lpe_audio_slock;
> > > +};
> > > +
> > > +#endif /* _I915_LPE_AUDIO_H_ */
> > > --
> > > 2.9.3
> > 
> > --
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI LPE audio driver

[Jani Nikula]

Jerome -

The same as last time, I didn't receive any of the patches, neither the
intel-gfx nor alsa-devel list archives have them, patchwork doesn't have
them, they're not in intel-gfx moderation queueu. I only have the
replies.

Please check your setup, and make sure your patches actually hit the
lists.

Thanks,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI LPE audio driver

[Anand, Jerome]

Jani,

Last time , it took close to 10 days when my patches were reflected on the gfx mailing list. Not sure what is the problem ?? Let me try sending it again ??

Regards,
Jerome

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Friday, November 25, 2016 4:22 PM
> To: Anand, Jerome <jerome.anand@intel.com>; Ville Syrjälä
> <ville.syrjala@linux.intel.com>
> Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> gfx@lists.freedesktop.org; broonie@kernel.org; Ughreja, Rakesh A
> <rakesh.a.ughreja@intel.com>
> Subject: Re: [Intel-gfx] [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI
> LPE audio driver
> 
> 
> Jerome -
> 
> The same as last time, I didn't receive any of the patches, neither the intel-
> gfx nor alsa-devel list archives have them, patchwork doesn't have them,
> they're not in intel-gfx moderation queueu. I only have the replies.
> 
> Please check your setup, and make sure your patches actually hit the lists.
> 
> Thanks,
> Jani.
> 
> 
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI LPE audio driver

[Jani Nikula]

On Mon, 28 Nov 2016, "Anand, Jerome" <jerome.anand@intel.com> wrote:
> Last time , it took close to 10 days when my patches were reflected on
> the gfx mailing list. Not sure what is the problem ?? Let me try
> sending it again ??

I guess the last time they only ever showed up because Ville bounced
them to the list after I'd wondered where the patches were...

Clearly the patches go through to people directly, at least Ville, and
your replies on the list go through, but there's got to be something
going on with how you send patches to the lists. (I'd suspect
freedesktop.org, but the patches weren't on alsa-devel archive either.)
You already seem to be using 'git send-email'. I don't know what to
suggest next, except to maybe try to send a test mail to the list using
'git send-email' and see if it goes through.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [alsa-devel] [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI LPE audio driver

[Pierre-Louis Bossart]

On 11/24/16 7:31 AM, Ville Syrjälä wrote:
>> +static void lpe_audio_irq_unmask(struct irq_data *d)
>> +{
>> +	struct drm_device *dev = d->chip_data;
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	unsigned long irqflags;
>> +	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
>> +		I915_LPE_PIPE_B_INTERRUPT |
>> +		I915_LPE_PIPE_C_INTERRUPT);
> No pipe C on vlv.

Yes but does it hurt to set this bit? If the hardware is not present 
then there is no side effect, is there?

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [alsa-devel] [RFC PATCH v3 1/7] drm/i915: setup bridge for HDMI LPE audio driver

[Ville Syrjälä]

On Sun, Nov 27, 2016 at 12:20:30PM -0600, Pierre-Louis Bossart wrote:
> On 11/24/16 7:31 AM, Ville Syrjälä wrote:
> >> +static void lpe_audio_irq_unmask(struct irq_data *d)
> >> +{
> >> +	struct drm_device *dev = d->chip_data;
> >> +	struct drm_i915_private *dev_priv = to_i915(dev);
> >> +	unsigned long irqflags;
> >> +	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> >> +		I915_LPE_PIPE_B_INTERRUPT |
> >> +		I915_LPE_PIPE_C_INTERRUPT);
> > No pipe C on vlv.
> 
> Yes but does it hurt to set this bit? If the hardware is not present 
> then there is no side effect, is there?

I'm too lazy to check if the bit is already taken.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

[Ville Syrjälä]

On Fri, Nov 25, 2016 at 05:25:43AM +0530, Jerome Anand wrote:
> Notifiations like mode change, hot plug and edid to
> the audio driver are added. This is inturn used by the
> audio driver for its functionality.
> 
> A new interface file capturing the notifications needed by the
> audio driver is added
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  3 +++
>  drivers/gpu/drm/i915/intel_audio.c     |  8 ++++++
>  drivers/gpu/drm/i915/intel_hdmi.c      |  1 +
>  drivers/gpu/drm/i915/intel_lpe_audio.c | 49 ++++++++++++++++++++++++++++++++++
>  include/drm/intel_lpe_audio.h          |  1 +
>  5 files changed, 62 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2a79048..33bc44c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3561,6 +3561,9 @@ int  intel_lpe_audio_setup(struct drm_i915_private *dev_priv);
>  void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv);
>  void intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv);
>  bool intel_lpe_audio_detect(struct drm_i915_private *dev_priv);
> +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> +			void *eld, int port, int tmds_clk_speed,
> +			bool connected);
>  
>  /* intel_i2c.c */
>  extern int intel_setup_gmbus(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 1c509f7..55a6831 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -24,6 +24,7 @@
>  #include <linux/kernel.h>
>  #include <linux/component.h>
>  #include <drm/i915_component.h>
> +#include <drm/intel_lpe_audio.h>
>  #include "intel_drv.h"
>  
>  #include <drm/drmP.h>
> @@ -627,6 +628,10 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
>  	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
>  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
>  						 (int) port, (int) pipe);
> +
> +	if (HAS_LPE_AUDIO(dev_priv))
> +		intel_lpe_audio_notify(dev_priv, connector->eld, port,
> +			crtc_state->port_clock, true);
>  }
>  
>  /**
> @@ -660,6 +665,9 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
>  	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
>  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
>  						 (int) port, (int) pipe);
> +
> +	if (HAS_LPE_AUDIO(dev_priv))
> +		intel_lpe_audio_notify(dev_priv, NULL, port, 0, true);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index fb88e32..02d50e3 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -36,6 +36,7 @@
>  #include <drm/drm_edid.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
> +#include <drm/intel_lpe_audio.h>
>  #include "i915_drv.h"
>  
>  static struct drm_device *intel_hdmi_to_dev(struct intel_hdmi *intel_hdmi)
> diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> index 5335fc6..93f83cb 100644
> --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> @@ -367,3 +367,52 @@ void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
> +
> +
> +/**
> + * intel_lpe_audio_notify() - notify lpe audio event
> + * audio driver and i915
> + * @dev_priv: the i915 drm device private data
> + * @eld : ELD data
> + * @port: port id
> + * @tmds_clk_speed: tmds clock frequency in Hz
> + * @connected: hdmi connected/disconnected
> + *
> + * Notify lpe audio driver of eld change.
> + */
> +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> +			void *eld, int port, int tmds_clk_speed,
> +			bool connected)
> +{
> +	unsigned long irq_flags;
> +
> +	if (HAS_LPE_AUDIO(dev_priv)) {
> +		struct intel_hdmi_lpe_audio_pdata *pdata = dev_get_platdata(
> +			&(dev_priv->lpe_audio.platdev->dev));
> +
> +		if (pdata) {

How could the !pdata case happen? If it can't we shouldn't cater for it.

> +			spin_lock_irqsave(&pdata->lpe_audio_slock,
> +				irq_flags);
> +
> +			if (eld != NULL) {
> +				memcpy(pdata->eld.eld_data, eld,
> +					HDMI_MAX_ELD_BYTES);
> +				pdata->eld.port_id = port;
> +
> +				if (tmds_clk_speed)

Why the if?

> +					pdata->tmds_clock_speed =
> +						tmds_clk_speed;
> +			}
> +			pdata->hdmi_connected = connected;

Also can't really see much need for this. Any one of the port/tmds
clock/eld should be sufficient.

> +			if (pdata->notify_audio_lpe)
> +				pdata->notify_audio_lpe(
> +					(eld != NULL) ? &pdata->eld : NULL);
> +			else
> +				pdata->notify_pending = true;

Still not sure why the "pending" thing is useful. Can't the audio driver
just do its thing (whatever it is) unconditionally?

When disabling just clear the port to INVALID, eld to zero, and tmds
clock to 0, and it should all be fine no?

> +
> +			spin_unlock_irqrestore(&pdata->lpe_audio_slock,
> +				irq_flags);
> +		} else
> +			DRM_DEBUG_DRIVER("no audio notification\n");
> +	}
> +}
> diff --git a/include/drm/intel_lpe_audio.h b/include/drm/intel_lpe_audio.h
> index a64c449..952de05 100644
> --- a/include/drm/intel_lpe_audio.h
> +++ b/include/drm/intel_lpe_audio.h
> @@ -25,6 +25,7 @@
>  #define _INTEL_LPE_AUDIO_H_
>  
>  #include <linux/types.h>
> +#include <linux/spinlock_types.h>
>  
>  #define HDMI_MAX_ELD_BYTES	128
>  
> -- 
> 2.9.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

[Anand, Jerome]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Thursday, November 24, 2016 7:03 PM
> To: Anand, Jerome <jerome.anand@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org;
> broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com;
> Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver
> notifications
> 
> On Fri, Nov 25, 2016 at 05:25:43AM +0530, Jerome Anand wrote:
> > Notifiations like mode change, hot plug and edid to the audio driver
> > are added. This is inturn used by the audio driver for its
> > functionality.
> >
> > A new interface file capturing the notifications needed by the audio
> > driver is added
> >
> > Signed-off-by: Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com>
> > Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h        |  3 +++
> >  drivers/gpu/drm/i915/intel_audio.c     |  8 ++++++
> >  drivers/gpu/drm/i915/intel_hdmi.c      |  1 +
> >  drivers/gpu/drm/i915/intel_lpe_audio.c | 49
> ++++++++++++++++++++++++++++++++++
> >  include/drm/intel_lpe_audio.h          |  1 +
> >  5 files changed, 62 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index 2a79048..33bc44c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3561,6 +3561,9 @@ int  intel_lpe_audio_setup(struct
> > drm_i915_private *dev_priv);  void intel_lpe_audio_teardown(struct
> > drm_i915_private *dev_priv);  void intel_lpe_audio_irq_handler(struct
> > drm_i915_private *dev_priv);  bool intel_lpe_audio_detect(struct
> > drm_i915_private *dev_priv);
> > +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > +			void *eld, int port, int tmds_clk_speed,
> > +			bool connected);
> >
> >  /* intel_i2c.c */
> >  extern int intel_setup_gmbus(struct drm_device *dev); diff --git
> > a/drivers/gpu/drm/i915/intel_audio.c
> > b/drivers/gpu/drm/i915/intel_audio.c
> > index 1c509f7..55a6831 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/component.h>
> >  #include <drm/i915_component.h>
> > +#include <drm/intel_lpe_audio.h>
> >  #include "intel_drv.h"
> >
> >  #include <drm/drmP.h>
> > @@ -627,6 +628,10 @@ void intel_audio_codec_enable(struct
> intel_encoder *intel_encoder,
> >  	if (acomp && acomp->audio_ops && acomp->audio_ops-
> >pin_eld_notify)
> >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> >audio_ptr,
> >  						 (int) port, (int) pipe);
> > +
> > +	if (HAS_LPE_AUDIO(dev_priv))
> > +		intel_lpe_audio_notify(dev_priv, connector->eld, port,
> > +			crtc_state->port_clock, true);
> >  }
> >
> >  /**
> > @@ -660,6 +665,9 @@ void intel_audio_codec_disable(struct
> intel_encoder *intel_encoder)
> >  	if (acomp && acomp->audio_ops && acomp->audio_ops-
> >pin_eld_notify)
> >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> >audio_ptr,
> >  						 (int) port, (int) pipe);
> > +
> > +	if (HAS_LPE_AUDIO(dev_priv))
> > +		intel_lpe_audio_notify(dev_priv, NULL, port, 0, true);
> >  }
> >
> >  /**
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index fb88e32..02d50e3 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -36,6 +36,7 @@
> >  #include <drm/drm_edid.h>
> >  #include "intel_drv.h"
> >  #include <drm/i915_drm.h>
> > +#include <drm/intel_lpe_audio.h>
> >  #include "i915_drv.h"
> >
> >  static struct drm_device *intel_hdmi_to_dev(struct intel_hdmi
> > *intel_hdmi) diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > index 5335fc6..93f83cb 100644
> > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > @@ -367,3 +367,52 @@ void intel_lpe_audio_teardown(struct
> > drm_i915_private *dev_priv)
> >
> >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);  }
> > +
> > +
> > +/**
> > + * intel_lpe_audio_notify() - notify lpe audio event
> > + * audio driver and i915
> > + * @dev_priv: the i915 drm device private data
> > + * @eld : ELD data
> > + * @port: port id
> > + * @tmds_clk_speed: tmds clock frequency in Hz
> > + * @connected: hdmi connected/disconnected
> > + *
> > + * Notify lpe audio driver of eld change.
> > + */
> > +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > +			void *eld, int port, int tmds_clk_speed,
> > +			bool connected)
> > +{
> > +	unsigned long irq_flags;
> > +
> > +	if (HAS_LPE_AUDIO(dev_priv)) {
> > +		struct intel_hdmi_lpe_audio_pdata *pdata =
> dev_get_platdata(
> > +			&(dev_priv->lpe_audio.platdev->dev));
> > +
> > +		if (pdata) {
> 
> How could the !pdata case happen? If it can't we shouldn't cater for it.
> 

It can happen if there is a notification before audio driver is initialized

> > +			spin_lock_irqsave(&pdata->lpe_audio_slock,
> > +				irq_flags);
> > +
> > +			if (eld != NULL) {
> > +				memcpy(pdata->eld.eld_data, eld,
> > +					HDMI_MAX_ELD_BYTES);
> > +				pdata->eld.port_id = port;
> > +
> > +				if (tmds_clk_speed)
> 
> Why the if?
> 

Tmds of zero is sent in codec_disable. Hence added, just to avoid unnecessary notification to audio driver

> > +					pdata->tmds_clock_speed =
> > +						tmds_clk_speed;
> > +			}
> > +			pdata->hdmi_connected = connected;
> 
> Also can't really see much need for this. Any one of the port/tmds clock/eld
> should be sufficient.

Eld and tmds are needed to cater to hotplug and display mode change notifications. Port can be avoided but kept for debug

> 
> > +			if (pdata->notify_audio_lpe)
> > +				pdata->notify_audio_lpe(
> > +					(eld != NULL) ? &pdata->eld : NULL);
> > +			else
> > +				pdata->notify_pending = true;
> 
> Still not sure why the "pending" thing is useful. Can't the audio driver just do
> its thing (whatever it is) unconditionally?
> 

This is added to avoid race when audio driver loads late and the notification from display has already passed.

> When disabling just clear the port to INVALID, eld to zero, and tmds clock to
> 0, and it should all be fine no?
> 

Yes, that's what is being done.

> > +
> > +			spin_unlock_irqrestore(&pdata->lpe_audio_slock,
> > +				irq_flags);
> > +		} else
> > +			DRM_DEBUG_DRIVER("no audio notification\n");
> > +	}
> > +}
> > diff --git a/include/drm/intel_lpe_audio.h
> > b/include/drm/intel_lpe_audio.h index a64c449..952de05 100644
> > --- a/include/drm/intel_lpe_audio.h
> > +++ b/include/drm/intel_lpe_audio.h
> > @@ -25,6 +25,7 @@
> >  #define _INTEL_LPE_AUDIO_H_
> >
> >  #include <linux/types.h>
> > +#include <linux/spinlock_types.h>
> >
> >  #define HDMI_MAX_ELD_BYTES	128
> >
> > --
> > 2.9.3
> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

[Ville Syrjälä]

On Fri, Nov 25, 2016 at 05:51:00AM +0000, Anand, Jerome wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Thursday, November 24, 2016 7:03 PM
> > To: Anand, Jerome <jerome.anand@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org;
> > broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com;
> > Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> > Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver
> > notifications
> > 
> > On Fri, Nov 25, 2016 at 05:25:43AM +0530, Jerome Anand wrote:
> > > Notifiations like mode change, hot plug and edid to the audio driver
> > > are added. This is inturn used by the audio driver for its
> > > functionality.
> > >
> > > A new interface file capturing the notifications needed by the audio
> > > driver is added
> > >
> > > Signed-off-by: Pierre-Louis Bossart
> > > <pierre-louis.bossart@linux.intel.com>
> > > Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h        |  3 +++
> > >  drivers/gpu/drm/i915/intel_audio.c     |  8 ++++++
> > >  drivers/gpu/drm/i915/intel_hdmi.c      |  1 +
> > >  drivers/gpu/drm/i915/intel_lpe_audio.c | 49
> > ++++++++++++++++++++++++++++++++++
> > >  include/drm/intel_lpe_audio.h          |  1 +
> > >  5 files changed, 62 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h index 2a79048..33bc44c 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -3561,6 +3561,9 @@ int  intel_lpe_audio_setup(struct
> > > drm_i915_private *dev_priv);  void intel_lpe_audio_teardown(struct
> > > drm_i915_private *dev_priv);  void intel_lpe_audio_irq_handler(struct
> > > drm_i915_private *dev_priv);  bool intel_lpe_audio_detect(struct
> > > drm_i915_private *dev_priv);
> > > +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > > +			void *eld, int port, int tmds_clk_speed,
> > > +			bool connected);
> > >
> > >  /* intel_i2c.c */
> > >  extern int intel_setup_gmbus(struct drm_device *dev); diff --git
> > > a/drivers/gpu/drm/i915/intel_audio.c
> > > b/drivers/gpu/drm/i915/intel_audio.c
> > > index 1c509f7..55a6831 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/kernel.h>
> > >  #include <linux/component.h>
> > >  #include <drm/i915_component.h>
> > > +#include <drm/intel_lpe_audio.h>
> > >  #include "intel_drv.h"
> > >
> > >  #include <drm/drmP.h>
> > > @@ -627,6 +628,10 @@ void intel_audio_codec_enable(struct
> > intel_encoder *intel_encoder,
> > >  	if (acomp && acomp->audio_ops && acomp->audio_ops-
> > >pin_eld_notify)
> > >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> > >audio_ptr,
> > >  						 (int) port, (int) pipe);
> > > +
> > > +	if (HAS_LPE_AUDIO(dev_priv))
> > > +		intel_lpe_audio_notify(dev_priv, connector->eld, port,
> > > +			crtc_state->port_clock, true);
> > >  }
> > >
> > >  /**
> > > @@ -660,6 +665,9 @@ void intel_audio_codec_disable(struct
> > intel_encoder *intel_encoder)
> > >  	if (acomp && acomp->audio_ops && acomp->audio_ops-
> > >pin_eld_notify)
> > >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> > >audio_ptr,
> > >  						 (int) port, (int) pipe);
> > > +
> > > +	if (HAS_LPE_AUDIO(dev_priv))
> > > +		intel_lpe_audio_notify(dev_priv, NULL, port, 0, true);
> > >  }
> > >
> > >  /**
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index fb88e32..02d50e3 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -36,6 +36,7 @@
> > >  #include <drm/drm_edid.h>
> > >  #include "intel_drv.h"
> > >  #include <drm/i915_drm.h>
> > > +#include <drm/intel_lpe_audio.h>
> > >  #include "i915_drv.h"
> > >
> > >  static struct drm_device *intel_hdmi_to_dev(struct intel_hdmi
> > > *intel_hdmi) diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > index 5335fc6..93f83cb 100644
> > > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > @@ -367,3 +367,52 @@ void intel_lpe_audio_teardown(struct
> > > drm_i915_private *dev_priv)
> > >
> > >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);  }
> > > +
> > > +
> > > +/**
> > > + * intel_lpe_audio_notify() - notify lpe audio event
> > > + * audio driver and i915
> > > + * @dev_priv: the i915 drm device private data
> > > + * @eld : ELD data
> > > + * @port: port id
> > > + * @tmds_clk_speed: tmds clock frequency in Hz
> > > + * @connected: hdmi connected/disconnected
> > > + *
> > > + * Notify lpe audio driver of eld change.
> > > + */
> > > +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > > +			void *eld, int port, int tmds_clk_speed,
> > > +			bool connected)
> > > +{
> > > +	unsigned long irq_flags;
> > > +
> > > +	if (HAS_LPE_AUDIO(dev_priv)) {
> > > +		struct intel_hdmi_lpe_audio_pdata *pdata =
> > dev_get_platdata(
> > > +			&(dev_priv->lpe_audio.platdev->dev));
> > > +
> > > +		if (pdata) {
> > 
> > How could the !pdata case happen? If it can't we shouldn't cater for it.
> > 
> 
> It can happen if there is a notification before audio driver is initialized

You're setting the platform_data in i915 code.

> 
> > > +			spin_lock_irqsave(&pdata->lpe_audio_slock,
> > > +				irq_flags);
> > > +
> > > +			if (eld != NULL) {
> > > +				memcpy(pdata->eld.eld_data, eld,
> > > +					HDMI_MAX_ELD_BYTES);
> > > +				pdata->eld.port_id = port;
> > > +
> > > +				if (tmds_clk_speed)
> > 
> > Why the if?
> > 
> 
> Tmds of zero is sent in codec_disable. Hence added, just to avoid unnecessary notification to audio driver

I have no idea why hanging on to a bogus tmds clock would prevert
some notification call.

> 
> > > +					pdata->tmds_clock_speed =
> > > +						tmds_clk_speed;
> > > +			}
> > > +			pdata->hdmi_connected = connected;
> > 
> > Also can't really see much need for this. Any one of the port/tmds clock/eld
> > should be sufficient.
> 
> Eld and tmds are needed to cater to hotplug and display mode change notifications. Port can be avoided but kept for debug

I don't know what you mean. The ELD/tmds clock should only be needed
when the display is on.

> 
> > 
> > > +			if (pdata->notify_audio_lpe)
> > > +				pdata->notify_audio_lpe(
> > > +					(eld != NULL) ? &pdata->eld : NULL);
> > > +			else
> > > +				pdata->notify_pending = true;
> > 
> > Still not sure why the "pending" thing is useful. Can't the audio driver just do
> > its thing (whatever it is) unconditionally?
> > 
> 
> This is added to avoid race when audio driver loads late and the notification from display has already passed.

You keep saying that but I can't see it.

> 
> > When disabling just clear the port to INVALID, eld to zero, and tmds clock to
> > 0, and it should all be fine no?
> > 
> 
> Yes, that's what is being done.

Where?

> 
> > > +
> > > +			spin_unlock_irqrestore(&pdata->lpe_audio_slock,
> > > +				irq_flags);
> > > +		} else
> > > +			DRM_DEBUG_DRIVER("no audio notification\n");
> > > +	}
> > > +}
> > > diff --git a/include/drm/intel_lpe_audio.h
> > > b/include/drm/intel_lpe_audio.h index a64c449..952de05 100644
> > > --- a/include/drm/intel_lpe_audio.h
> > > +++ b/include/drm/intel_lpe_audio.h
> > > @@ -25,6 +25,7 @@
> > >  #define _INTEL_LPE_AUDIO_H_
> > >
> > >  #include <linux/types.h>
> > > +#include <linux/spinlock_types.h>
> > >
> > >  #define HDMI_MAX_ELD_BYTES	128
> > >
> > > --
> > > 2.9.3
> > 
> > --
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

[Anand, Jerome]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Monday, November 28, 2016 7:30 PM
> To: Anand, Jerome <jerome.anand@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org;
> broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com;
> Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver
> notifications
> 
> On Fri, Nov 25, 2016 at 05:51:00AM +0000, Anand, Jerome wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Thursday, November 24, 2016 7:03 PM
> > > To: Anand, Jerome <jerome.anand@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org;
> > > broonie@kernel.org; tiwai@suse.de;
> > > pierre-louis.bossart@linux.intel.com;
> > > Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> > > Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio
> > > driver notifications
> > >
> > > On Fri, Nov 25, 2016 at 05:25:43AM +0530, Jerome Anand wrote:
> > > > Notifiations like mode change, hot plug and edid to the audio
> > > > driver are added. This is inturn used by the audio driver for its
> > > > functionality.
> > > >
> > > > A new interface file capturing the notifications needed by the
> > > > audio driver is added
> > > >
> > > > Signed-off-by: Pierre-Louis Bossart
> > > > <pierre-louis.bossart@linux.intel.com>
> > > > Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h        |  3 +++
> > > >  drivers/gpu/drm/i915/intel_audio.c     |  8 ++++++
> > > >  drivers/gpu/drm/i915/intel_hdmi.c      |  1 +
> > > >  drivers/gpu/drm/i915/intel_lpe_audio.c | 49
> > > ++++++++++++++++++++++++++++++++++
> > > >  include/drm/intel_lpe_audio.h          |  1 +
> > > >  5 files changed, 62 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h index 2a79048..33bc44c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -3561,6 +3561,9 @@ int  intel_lpe_audio_setup(struct
> > > > drm_i915_private *dev_priv);  void intel_lpe_audio_teardown(struct
> > > > drm_i915_private *dev_priv);  void
> > > > intel_lpe_audio_irq_handler(struct
> > > > drm_i915_private *dev_priv);  bool intel_lpe_audio_detect(struct
> > > > drm_i915_private *dev_priv);
> > > > +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > > > +			void *eld, int port, int tmds_clk_speed,
> > > > +			bool connected);
> > > >
> > > >  /* intel_i2c.c */
> > > >  extern int intel_setup_gmbus(struct drm_device *dev); diff --git
> > > > a/drivers/gpu/drm/i915/intel_audio.c
> > > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > index 1c509f7..55a6831 100644
> > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > @@ -24,6 +24,7 @@
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/component.h>
> > > >  #include <drm/i915_component.h>
> > > > +#include <drm/intel_lpe_audio.h>
> > > >  #include "intel_drv.h"
> > > >
> > > >  #include <drm/drmP.h>
> > > > @@ -627,6 +628,10 @@ void intel_audio_codec_enable(struct
> > > intel_encoder *intel_encoder,
> > > >  	if (acomp && acomp->audio_ops && acomp->audio_ops-
> > > >pin_eld_notify)
> > > >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> > > >audio_ptr,
> > > >  						 (int) port, (int) pipe);
> > > > +
> > > > +	if (HAS_LPE_AUDIO(dev_priv))
> > > > +		intel_lpe_audio_notify(dev_priv, connector->eld, port,
> > > > +			crtc_state->port_clock, true);
> > > >  }
> > > >
> > > >  /**
> > > > @@ -660,6 +665,9 @@ void intel_audio_codec_disable(struct
> > > intel_encoder *intel_encoder)
> > > >  	if (acomp && acomp->audio_ops && acomp->audio_ops-
> > > >pin_eld_notify)
> > > >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> > > >audio_ptr,
> > > >  						 (int) port, (int) pipe);
> > > > +
> > > > +	if (HAS_LPE_AUDIO(dev_priv))
> > > > +		intel_lpe_audio_notify(dev_priv, NULL, port, 0, true);
> > > >  }
> > > >
> > > >  /**
> > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > index fb88e32..02d50e3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > @@ -36,6 +36,7 @@
> > > >  #include <drm/drm_edid.h>
> > > >  #include "intel_drv.h"
> > > >  #include <drm/i915_drm.h>
> > > > +#include <drm/intel_lpe_audio.h>
> > > >  #include "i915_drv.h"
> > > >
> > > >  static struct drm_device *intel_hdmi_to_dev(struct intel_hdmi
> > > > *intel_hdmi) diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > index 5335fc6..93f83cb 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > @@ -367,3 +367,52 @@ void intel_lpe_audio_teardown(struct
> > > > drm_i915_private *dev_priv)
> > > >
> > > >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);  }
> > > > +
> > > > +
> > > > +/**
> > > > + * intel_lpe_audio_notify() - notify lpe audio event
> > > > + * audio driver and i915
> > > > + * @dev_priv: the i915 drm device private data
> > > > + * @eld : ELD data
> > > > + * @port: port id
> > > > + * @tmds_clk_speed: tmds clock frequency in Hz
> > > > + * @connected: hdmi connected/disconnected
> > > > + *
> > > > + * Notify lpe audio driver of eld change.
> > > > + */
> > > > +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > > > +			void *eld, int port, int tmds_clk_speed,
> > > > +			bool connected)
> > > > +{
> > > > +	unsigned long irq_flags;
> > > > +
> > > > +	if (HAS_LPE_AUDIO(dev_priv)) {
> > > > +		struct intel_hdmi_lpe_audio_pdata *pdata =
> > > dev_get_platdata(
> > > > +			&(dev_priv->lpe_audio.platdev->dev));
> > > > +
> > > > +		if (pdata) {
> > >
> > > How could the !pdata case happen? If it can't we shouldn't cater for it.
> > >
> >
> > It can happen if there is a notification before audio driver is
> > initialized
> 
> You're setting the platform_data in i915 code.
> 

OK

> >
> > > > +			spin_lock_irqsave(&pdata->lpe_audio_slock,
> > > > +				irq_flags);
> > > > +
> > > > +			if (eld != NULL) {
> > > > +				memcpy(pdata->eld.eld_data, eld,
> > > > +					HDMI_MAX_ELD_BYTES);
> > > > +				pdata->eld.port_id = port;
> > > > +
> > > > +				if (tmds_clk_speed)
> > >
> > > Why the if?
> > >
> >
> > Tmds of zero is sent in codec_disable. Hence added, just to avoid
> > unnecessary notification to audio driver
> 
> I have no idea why hanging on to a bogus tmds clock would prevert some
> notification call.
> 

Am not sure what advantage is gained here by removing it

> >
> > > > +					pdata->tmds_clock_speed =
> > > > +						tmds_clk_speed;
> > > > +			}
> > > > +			pdata->hdmi_connected = connected;
> > >
> > > Also can't really see much need for this. Any one of the port/tmds
> > > clock/eld should be sufficient.
> >
> > Eld and tmds are needed to cater to hotplug and display mode change
> > notifications. Port can be avoided but kept for debug
> 
> I don't know what you mean. The ELD/tmds clock should only be needed
> when the display is on.

I don't get your point. We are doing this whole exercise to cater to display ON scenarios.

> 
> >
> > >
> > > > +			if (pdata->notify_audio_lpe)
> > > > +				pdata->notify_audio_lpe(
> > > > +					(eld != NULL) ? &pdata->eld : NULL);
> > > > +			else
> > > > +				pdata->notify_pending = true;
> > >
> > > Still not sure why the "pending" thing is useful. Can't the audio
> > > driver just do its thing (whatever it is) unconditionally?
> > >
> >
> > This is added to avoid race when audio driver loads late and the notification
> from display has already passed.
> 
> You keep saying that but I can't see it.
> 

I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver. 
Since the audio callbacks are not initialized, notification gets missed.

> >
> > > When disabling just clear the port to INVALID, eld to zero, and tmds
> > > clock to 0, and it should all be fine no?
> > >
> >
> > Yes, that's what is being done.
> 
> Where?
> 

Notify callback will have eld to NULL and tmds to zero sent in codec_disable

> >
> > > > +
> > > > +			spin_unlock_irqrestore(&pdata->lpe_audio_slock,
> > > > +				irq_flags);
> > > > +		} else
> > > > +			DRM_DEBUG_DRIVER("no audio notification\n");
> > > > +	}
> > > > +}
> > > > diff --git a/include/drm/intel_lpe_audio.h
> > > > b/include/drm/intel_lpe_audio.h index a64c449..952de05 100644
> > > > --- a/include/drm/intel_lpe_audio.h
> > > > +++ b/include/drm/intel_lpe_audio.h
> > > > @@ -25,6 +25,7 @@
> > > >  #define _INTEL_LPE_AUDIO_H_
> > > >
> > > >  #include <linux/types.h>
> > > > +#include <linux/spinlock_types.h>
> > > >
> > > >  #define HDMI_MAX_ELD_BYTES	128
> > > >
> > > > --
> > > > 2.9.3
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

[Ville Syrjälä]

On Mon, Nov 28, 2016 at 04:50:15PM +0000, Anand, Jerome wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Monday, November 28, 2016 7:30 PM
> > To: Anand, Jerome <jerome.anand@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org;
> > broonie@kernel.org; tiwai@suse.de; pierre-louis.bossart@linux.intel.com;
> > Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> > Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver
> > notifications
> > 
> > On Fri, Nov 25, 2016 at 05:51:00AM +0000, Anand, Jerome wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > Sent: Thursday, November 24, 2016 7:03 PM
> > > > To: Anand, Jerome <jerome.anand@intel.com>
> > > > Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org;
> > > > broonie@kernel.org; tiwai@suse.de;
> > > > pierre-louis.bossart@linux.intel.com;
> > > > Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>
> > > > Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio
> > > > driver notifications
> > > >
> > > > On Fri, Nov 25, 2016 at 05:25:43AM +0530, Jerome Anand wrote:
> > > > > Notifiations like mode change, hot plug and edid to the audio
> > > > > driver are added. This is inturn used by the audio driver for its
> > > > > functionality.
> > > > >
> > > > > A new interface file capturing the notifications needed by the
> > > > > audio driver is added
> > > > >
> > > > > Signed-off-by: Pierre-Louis Bossart
> > > > > <pierre-louis.bossart@linux.intel.com>
> > > > > Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.h        |  3 +++
> > > > >  drivers/gpu/drm/i915/intel_audio.c     |  8 ++++++
> > > > >  drivers/gpu/drm/i915/intel_hdmi.c      |  1 +
> > > > >  drivers/gpu/drm/i915/intel_lpe_audio.c | 49
> > > > ++++++++++++++++++++++++++++++++++
> > > > >  include/drm/intel_lpe_audio.h          |  1 +
> > > > >  5 files changed, 62 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > b/drivers/gpu/drm/i915/i915_drv.h index 2a79048..33bc44c 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -3561,6 +3561,9 @@ int  intel_lpe_audio_setup(struct
> > > > > drm_i915_private *dev_priv);  void intel_lpe_audio_teardown(struct
> > > > > drm_i915_private *dev_priv);  void
> > > > > intel_lpe_audio_irq_handler(struct
> > > > > drm_i915_private *dev_priv);  bool intel_lpe_audio_detect(struct
> > > > > drm_i915_private *dev_priv);
> > > > > +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > > > > +			void *eld, int port, int tmds_clk_speed,
> > > > > +			bool connected);
> > > > >
> > > > >  /* intel_i2c.c */
> > > > >  extern int intel_setup_gmbus(struct drm_device *dev); diff --git
> > > > > a/drivers/gpu/drm/i915/intel_audio.c
> > > > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > > index 1c509f7..55a6831 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > @@ -24,6 +24,7 @@
> > > > >  #include <linux/kernel.h>
> > > > >  #include <linux/component.h>
> > > > >  #include <drm/i915_component.h>
> > > > > +#include <drm/intel_lpe_audio.h>
> > > > >  #include "intel_drv.h"
> > > > >
> > > > >  #include <drm/drmP.h>
> > > > > @@ -627,6 +628,10 @@ void intel_audio_codec_enable(struct
> > > > intel_encoder *intel_encoder,
> > > > >  	if (acomp && acomp->audio_ops && acomp->audio_ops-
> > > > >pin_eld_notify)
> > > > >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> > > > >audio_ptr,
> > > > >  						 (int) port, (int) pipe);
> > > > > +
> > > > > +	if (HAS_LPE_AUDIO(dev_priv))
> > > > > +		intel_lpe_audio_notify(dev_priv, connector->eld, port,
> > > > > +			crtc_state->port_clock, true);
> > > > >  }
> > > > >
> > > > >  /**
> > > > > @@ -660,6 +665,9 @@ void intel_audio_codec_disable(struct
> > > > intel_encoder *intel_encoder)
> > > > >  	if (acomp && acomp->audio_ops && acomp->audio_ops-
> > > > >pin_eld_notify)
> > > > >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> > > > >audio_ptr,
> > > > >  						 (int) port, (int) pipe);
> > > > > +
> > > > > +	if (HAS_LPE_AUDIO(dev_priv))
> > > > > +		intel_lpe_audio_notify(dev_priv, NULL, port, 0, true);
> > > > >  }
> > > > >
> > > > >  /**
> > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > index fb88e32..02d50e3 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > @@ -36,6 +36,7 @@
> > > > >  #include <drm/drm_edid.h>
> > > > >  #include "intel_drv.h"
> > > > >  #include <drm/i915_drm.h>
> > > > > +#include <drm/intel_lpe_audio.h>
> > > > >  #include "i915_drv.h"
> > > > >
> > > > >  static struct drm_device *intel_hdmi_to_dev(struct intel_hdmi
> > > > > *intel_hdmi) diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > > b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > > index 5335fc6..93f83cb 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > > @@ -367,3 +367,52 @@ void intel_lpe_audio_teardown(struct
> > > > > drm_i915_private *dev_priv)
> > > > >
> > > > >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);  }
> > > > > +
> > > > > +
> > > > > +/**
> > > > > + * intel_lpe_audio_notify() - notify lpe audio event
> > > > > + * audio driver and i915
> > > > > + * @dev_priv: the i915 drm device private data
> > > > > + * @eld : ELD data
> > > > > + * @port: port id
> > > > > + * @tmds_clk_speed: tmds clock frequency in Hz
> > > > > + * @connected: hdmi connected/disconnected
> > > > > + *
> > > > > + * Notify lpe audio driver of eld change.
> > > > > + */
> > > > > +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > > > > +			void *eld, int port, int tmds_clk_speed,
> > > > > +			bool connected)
> > > > > +{
> > > > > +	unsigned long irq_flags;
> > > > > +
> > > > > +	if (HAS_LPE_AUDIO(dev_priv)) {
> > > > > +		struct intel_hdmi_lpe_audio_pdata *pdata =
> > > > dev_get_platdata(
> > > > > +			&(dev_priv->lpe_audio.platdev->dev));
> > > > > +
> > > > > +		if (pdata) {
> > > >
> > > > How could the !pdata case happen? If it can't we shouldn't cater for it.
> > > >
> > >
> > > It can happen if there is a notification before audio driver is
> > > initialized
> > 
> > You're setting the platform_data in i915 code.
> > 
> 
> OK
> 
> > >
> > > > > +			spin_lock_irqsave(&pdata->lpe_audio_slock,
> > > > > +				irq_flags);
> > > > > +
> > > > > +			if (eld != NULL) {
> > > > > +				memcpy(pdata->eld.eld_data, eld,
> > > > > +					HDMI_MAX_ELD_BYTES);
> > > > > +				pdata->eld.port_id = port;
> > > > > +
> > > > > +				if (tmds_clk_speed)
> > > >
> > > > Why the if?
> > > >
> > >
> > > Tmds of zero is sent in codec_disable. Hence added, just to avoid
> > > unnecessary notification to audio driver
> > 
> > I have no idea why hanging on to a bogus tmds clock would prevert some
> > notification call.
> > 
> 
> Am not sure what advantage is gained here by removing it
> 
> > >
> > > > > +					pdata->tmds_clock_speed =
> > > > > +						tmds_clk_speed;
> > > > > +			}
> > > > > +			pdata->hdmi_connected = connected;
> > > >
> > > > Also can't really see much need for this. Any one of the port/tmds
> > > > clock/eld should be sufficient.
> > >
> > > Eld and tmds are needed to cater to hotplug and display mode change
> > > notifications. Port can be avoided but kept for debug
> > 
> > I don't know what you mean. The ELD/tmds clock should only be needed
> > when the display is on.
> 
> I don't get your point. We are doing this whole exercise to cater to display ON scenarios.

Yes, but the hdmi_connected thing seems totally redundant. It can be
derived from any of the eld/tmds_clock/port being set to 0/INVALID.

> 
> > 
> > >
> > > >
> > > > > +			if (pdata->notify_audio_lpe)
> > > > > +				pdata->notify_audio_lpe(
> > > > > +					(eld != NULL) ? &pdata->eld : NULL);
> > > > > +			else
> > > > > +				pdata->notify_pending = true;
> > > >
> > > > Still not sure why the "pending" thing is useful. Can't the audio
> > > > driver just do its thing (whatever it is) unconditionally?
> > > >
> > >
> > > This is added to avoid race when audio driver loads late and the notification
> > from display has already passed.
> > 
> > You keep saying that but I can't see it.
> > 
> 
> I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver. 
> Since the audio callbacks are not initialized, notification gets missed.

Sure. But what does the extra notification_pending flag buy us? The
audio driver could just check the eld/tmds_clock/port directly.

> 
> > >
> > > > When disabling just clear the port to INVALID, eld to zero, and tmds
> > > > clock to 0, and it should all be fine no?
> > > >
> > >
> > > Yes, that's what is being done.
> > 
> > Where?
> > 
> 
> Notify callback will have eld to NULL and tmds to zero sent in codec_disable

But the driver can look those thigns up directly as well it seems. So
this whole thing is a bit of a mess on account of sharing the platform
as a communication channel and also trying to pass the things as
paraameters to the notify hook. I think we need to pick one or the other
approach, not some mismash of both.

> 
> > >
> > > > > +
> > > > > +			spin_unlock_irqrestore(&pdata->lpe_audio_slock,
> > > > > +				irq_flags);
> > > > > +		} else
> > > > > +			DRM_DEBUG_DRIVER("no audio notification\n");
> > > > > +	}
> > > > > +}
> > > > > diff --git a/include/drm/intel_lpe_audio.h
> > > > > b/include/drm/intel_lpe_audio.h index a64c449..952de05 100644
> > > > > --- a/include/drm/intel_lpe_audio.h
> > > > > +++ b/include/drm/intel_lpe_audio.h
> > > > > @@ -25,6 +25,7 @@
> > > > >  #define _INTEL_LPE_AUDIO_H_
> > > > >
> > > > >  #include <linux/types.h>
> > > > > +#include <linux/spinlock_types.h>
> > > > >
> > > > >  #define HDMI_MAX_ELD_BYTES	128
> > > > >
> > > > > --
> > > > > 2.9.3
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> > 
> > --
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

[Pierre-Louis Bossart]

On 11/28/16 11:01 AM, Ville Syrjälä wrote:
>>>>>> +			if (pdata->notify_audio_lpe)
>>>>>> +				pdata->notify_audio_lpe(
>>>>>> +					(eld != NULL) ? &pdata->eld : NULL);
>>>>>> +			else
>>>>>> +				pdata->notify_pending = true;
>>>>> Still not sure why the "pending" thing is useful. Can't the audio
>>>>> driver just do its thing (whatever it is) unconditionally?
>>>>>
>>>> This is added to avoid race when audio driver loads late and the notification
>>> from display has already passed.
>>>
>>> You keep saying that but I can't see it.
>>>
>> I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver.
>> Since the audio callbacks are not initialized, notification gets missed.
> Sure. But what does the extra notification_pending flag buy us? The
> audio driver could just check the eld/tmds_clock/port directly.
>
>>>>> When disabling just clear the port to INVALID, eld to zero, and tmds
>>>>> clock to 0, and it should all be fine no?
>>>>>
>>>> Yes, that's what is being done.
>>> Where?
>>>
>> Notify callback will have eld to NULL and tmds to zero sent in codec_disable
> But the driver can look those thigns up directly as well it seems. So
> this whole thing is a bit of a mess on account of sharing the platform
> as a communication channel and also trying to pass the things as
> paraameters to the notify hook. I think we need to pick one or the other
> approach, not some mismash of both.

Indeed it looks weird to have both a parameter for tmds_clock in the 
pdata AND the notify parameter, this can probably be cleaned-up.

That said, I am not sure I completely understand the feedback that the 
audio driver can get all the eld/tmds/port information directly. We are 
trying to avoid accessing the data structures of the i915 driver. Are 
you suggesting a scheme where the i915 driver would just provide a 
door-bell like notification and the audio driver would use a 
get_eld/tmds/port interface exposed by the i915 driver on startup and 
upon receiving this notification?


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

[Ville Syrjälä]

On Mon, Nov 28, 2016 at 01:13:31PM -0600, Pierre-Louis Bossart wrote:
> 
> On 11/28/16 11:01 AM, Ville Syrjälä wrote:
> >>>>>> +			if (pdata->notify_audio_lpe)
> >>>>>> +				pdata->notify_audio_lpe(
> >>>>>> +					(eld != NULL) ? &pdata->eld : NULL);
> >>>>>> +			else
> >>>>>> +				pdata->notify_pending = true;
> >>>>> Still not sure why the "pending" thing is useful. Can't the audio
> >>>>> driver just do its thing (whatever it is) unconditionally?
> >>>>>
> >>>> This is added to avoid race when audio driver loads late and the notification
> >>> from display has already passed.
> >>>
> >>> You keep saying that but I can't see it.
> >>>
> >> I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver.
> >> Since the audio callbacks are not initialized, notification gets missed.
> > Sure. But what does the extra notification_pending flag buy us? The
> > audio driver could just check the eld/tmds_clock/port directly.
> >
> >>>>> When disabling just clear the port to INVALID, eld to zero, and tmds
> >>>>> clock to 0, and it should all be fine no?
> >>>>>
> >>>> Yes, that's what is being done.
> >>> Where?
> >>>
> >> Notify callback will have eld to NULL and tmds to zero sent in codec_disable
> > But the driver can look those thigns up directly as well it seems. So
> > this whole thing is a bit of a mess on account of sharing the platform
> > as a communication channel and also trying to pass the things as
> > paraameters to the notify hook. I think we need to pick one or the other
> > approach, not some mismash of both.
> 
> Indeed it looks weird to have both a parameter for tmds_clock in the 
> pdata AND the notify parameter, this can probably be cleaned-up.
> 
> That said, I am not sure I completely understand the feedback that the 
> audio driver can get all the eld/tmds/port information directly. We are 
> trying to avoid accessing the data structures of the i915 driver.

IIRC what I proposed originally didn't even expose the same structure
to both sides, but that's not what we seem to have atm.

> Are 
> you suggesting a scheme where the i915 driver would just provide a 
> door-bell like notification and the audio driver would use a 
> get_eld/tmds/port interface exposed by the i915 driver on startup and 
> upon receiving this notification?
> 

Well, we could do it that way, or we'd do it the other way that the
audio driver just calls i915 to triggers a single i915->audio notify
call after the audio driver has finished its probe. Or we could
do whatever we seem to have now is where the audio driver can just
root around directly in the structure (at which point passing any
parameters in the notify calls seems redundant as well).

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

[Pierre-Louis Bossart]

On 11/28/16 1:30 PM, Ville Syrjälä wrote:
> On Mon, Nov 28, 2016 at 01:13:31PM -0600, Pierre-Louis Bossart wrote:
>>
>> On 11/28/16 11:01 AM, Ville Syrjälä wrote:
>>>>>>>> +			if (pdata->notify_audio_lpe)
>>>>>>>> +				pdata->notify_audio_lpe(
>>>>>>>> +					(eld != NULL) ? &pdata->eld : NULL);
>>>>>>>> +			else
>>>>>>>> +				pdata->notify_pending = true;
>>>>>>> Still not sure why the "pending" thing is useful. Can't the audio
>>>>>>> driver just do its thing (whatever it is) unconditionally?
>>>>>>>
>>>>>> This is added to avoid race when audio driver loads late and the notification
>>>>> from display has already passed.
>>>>>
>>>>> You keep saying that but I can't see it.
>>>>>
>>>> I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver.
>>>> Since the audio callbacks are not initialized, notification gets missed.
>>> Sure. But what does the extra notification_pending flag buy us? The
>>> audio driver could just check the eld/tmds_clock/port directly.
>>>
>>>>>>> When disabling just clear the port to INVALID, eld to zero, and tmds
>>>>>>> clock to 0, and it should all be fine no?
>>>>>>>
>>>>>> Yes, that's what is being done.
>>>>> Where?
>>>>>
>>>> Notify callback will have eld to NULL and tmds to zero sent in codec_disable
>>> But the driver can look those thigns up directly as well it seems. So
>>> this whole thing is a bit of a mess on account of sharing the platform
>>> as a communication channel and also trying to pass the things as
>>> paraameters to the notify hook. I think we need to pick one or the other
>>> approach, not some mismash of both.
>>
>> Indeed it looks weird to have both a parameter for tmds_clock in the
>> pdata AND the notify parameter, this can probably be cleaned-up.
>>
>> That said, I am not sure I completely understand the feedback that the
>> audio driver can get all the eld/tmds/port information directly. We are
>> trying to avoid accessing the data structures of the i915 driver.
>
> IIRC what I proposed originally didn't even expose the same structure
> to both sides, but that's not what we seem to have atm.
>
>> Are
>> you suggesting a scheme where the i915 driver would just provide a
>> door-bell like notification and the audio driver would use a
>> get_eld/tmds/port interface exposed by the i915 driver on startup and
>> upon receiving this notification?
>>
>
> Well, we could do it that way, or we'd do it the other way that the
> audio driver just calls i915 to triggers a single i915->audio notify
> call after the audio driver has finished its probe. Or we could
> do whatever we seem to have now is where the audio driver can just
> root around directly in the structure (at which point passing any
> parameters in the notify calls seems redundant as well).

Looking at some older emails, i think you recommended a 'register' 
callback to let the audio driver signal to the i915 side it completed 
its initialization, with a single notify generated if needed (what you 
described just above as 'the other way')

If you look at the path 4 of the series, Jerome was trying to implement 
this with a 'notify_pending' field in the platform data set by the i915 
side and used during the audio driver probe

+	if (pdata->notify_pending) {
+
+		pr_debug("%s: handle pending notification\n", __func__);
+		notify_audio_lpe(&pdata->eld);
+		pdata->notify_pending = false;
+	}

Maybe an explicit handshake is more self-explanatory and safer?

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

[Takashi Iwai]

On Mon, 28 Nov 2016 22:56:24 +0100,
Pierre-Louis Bossart wrote:
> 
> On 11/28/16 1:30 PM, Ville Syrjälä wrote:
> > On Mon, Nov 28, 2016 at 01:13:31PM -0600, Pierre-Louis Bossart wrote:
> >>
> >> On 11/28/16 11:01 AM, Ville Syrjälä wrote:
> >>>>>>>> +			if (pdata->notify_audio_lpe)
> >>>>>>>> +				pdata->notify_audio_lpe(
> >>>>>>>> +					(eld != NULL) ? &pdata->eld : NULL);
> >>>>>>>> +			else
> >>>>>>>> +				pdata->notify_pending = true;
> >>>>>>> Still not sure why the "pending" thing is useful. Can't the audio
> >>>>>>> driver just do its thing (whatever it is) unconditionally?
> >>>>>>>
> >>>>>> This is added to avoid race when audio driver loads late and the notification
> >>>>> from display has already passed.
> >>>>>
> >>>>> You keep saying that but I can't see it.
> >>>>>
> >>>> I have seen this happen - before audio driver is loaded, codec enable completes and notification is sent to the audio driver.
> >>>> Since the audio callbacks are not initialized, notification gets missed.
> >>> Sure. But what does the extra notification_pending flag buy us? The
> >>> audio driver could just check the eld/tmds_clock/port directly.
> >>>
> >>>>>>> When disabling just clear the port to INVALID, eld to zero, and tmds
> >>>>>>> clock to 0, and it should all be fine no?
> >>>>>>>
> >>>>>> Yes, that's what is being done.
> >>>>> Where?
> >>>>>
> >>>> Notify callback will have eld to NULL and tmds to zero sent in codec_disable
> >>> But the driver can look those thigns up directly as well it seems. So
> >>> this whole thing is a bit of a mess on account of sharing the platform
> >>> as a communication channel and also trying to pass the things as
> >>> paraameters to the notify hook. I think we need to pick one or the other
> >>> approach, not some mismash of both.
> >>
> >> Indeed it looks weird to have both a parameter for tmds_clock in the
> >> pdata AND the notify parameter, this can probably be cleaned-up.
> >>
> >> That said, I am not sure I completely understand the feedback that the
> >> audio driver can get all the eld/tmds/port information directly. We are
> >> trying to avoid accessing the data structures of the i915 driver.
> >
> > IIRC what I proposed originally didn't even expose the same structure
> > to both sides, but that's not what we seem to have atm.
> >
> >> Are
> >> you suggesting a scheme where the i915 driver would just provide a
> >> door-bell like notification and the audio driver would use a
> >> get_eld/tmds/port interface exposed by the i915 driver on startup and
> >> upon receiving this notification?
> >>
> >
> > Well, we could do it that way, or we'd do it the other way that the
> > audio driver just calls i915 to triggers a single i915->audio notify
> > call after the audio driver has finished its probe. Or we could
> > do whatever we seem to have now is where the audio driver can just
> > root around directly in the structure (at which point passing any
> > parameters in the notify calls seems redundant as well).
> 
> Looking at some older emails, i think you recommended a 'register'
> callback to let the audio driver signal to the i915 side it completed
> its initialization, with a single notify generated if needed (what you
> described just above as 'the other way')
> 
> If you look at the path 4 of the series,

I seem to have missed the whole series by some reason, both to my
inbox and to ML.  Could you resubmit later?


> Jerome was trying to
> implement this with a 'notify_pending' field in the platform data set
> by the i915 side and used during the audio driver probe
> 
> +	if (pdata->notify_pending) {
> +
> +		pr_debug("%s: handle pending notification\n", __func__);
> +		notify_audio_lpe(&pdata->eld);
> +		pdata->notify_pending = false;
> +	}
> 
> Maybe an explicit handshake is more self-explanatory and safer?

IMO yes, the pending notification flag isn't intrusive.

There are cases where the audio driver wants to inquire the status
from gfx side; at least, the HD-audio driver needed to recheck after
PM.

Also, isn't the code above racy without a proper mutex?


thanks,

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

Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

[Anand, Jerome]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, November 29, 2016 1:58 PM
> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>; Anand, Jerome
> <jerome.anand@intel.com>; intel-gfx@lists.freedesktop.org; alsa-
> devel@alsa-project.org; broonie@kernel.org; Ughreja, Rakesh A
> <rakesh.a.ughreja@intel.com>
> Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver
> notifications
> 
> On Mon, 28 Nov 2016 22:56:24 +0100,
> Pierre-Louis Bossart wrote:
> >
> > On 11/28/16 1:30 PM, Ville Syrjälä wrote:
> > > On Mon, Nov 28, 2016 at 01:13:31PM -0600, Pierre-Louis Bossart wrote:
> > >>
> > >> On 11/28/16 11:01 AM, Ville Syrjälä wrote:
> > >>>>>>>> +			if (pdata->notify_audio_lpe)
> > >>>>>>>> +				pdata->notify_audio_lpe(
> > >>>>>>>> +					(eld != NULL) ? &pdata->eld :
> NULL);
> > >>>>>>>> +			else
> > >>>>>>>> +				pdata->notify_pending = true;
> > >>>>>>> Still not sure why the "pending" thing is useful. Can't the
> > >>>>>>> audio driver just do its thing (whatever it is) unconditionally?
> > >>>>>>>
> > >>>>>> This is added to avoid race when audio driver loads late and
> > >>>>>> the notification
> > >>>>> from display has already passed.
> > >>>>>
> > >>>>> You keep saying that but I can't see it.
> > >>>>>
> > >>>> I have seen this happen - before audio driver is loaded, codec enable
> completes and notification is sent to the audio driver.
> > >>>> Since the audio callbacks are not initialized, notification gets missed.
> > >>> Sure. But what does the extra notification_pending flag buy us?
> > >>> The audio driver could just check the eld/tmds_clock/port directly.
> > >>>
> > >>>>>>> When disabling just clear the port to INVALID, eld to zero,
> > >>>>>>> and tmds clock to 0, and it should all be fine no?
> > >>>>>>>
> > >>>>>> Yes, that's what is being done.
> > >>>>> Where?
> > >>>>>
> > >>>> Notify callback will have eld to NULL and tmds to zero sent in
> > >>>> codec_disable
> > >>> But the driver can look those thigns up directly as well it seems.
> > >>> So this whole thing is a bit of a mess on account of sharing the
> > >>> platform as a communication channel and also trying to pass the
> > >>> things as paraameters to the notify hook. I think we need to pick
> > >>> one or the other approach, not some mismash of both.
> > >>
> > >> Indeed it looks weird to have both a parameter for tmds_clock in
> > >> the pdata AND the notify parameter, this can probably be cleaned-up.
> > >>
> > >> That said, I am not sure I completely understand the feedback that
> > >> the audio driver can get all the eld/tmds/port information
> > >> directly. We are trying to avoid accessing the data structures of the i915
> driver.
> > >
> > > IIRC what I proposed originally didn't even expose the same
> > > structure to both sides, but that's not what we seem to have atm.
> > >
> > >> Are
> > >> you suggesting a scheme where the i915 driver would just provide a
> > >> door-bell like notification and the audio driver would use a
> > >> get_eld/tmds/port interface exposed by the i915 driver on startup
> > >> and upon receiving this notification?
> > >>
> > >
> > > Well, we could do it that way, or we'd do it the other way that the
> > > audio driver just calls i915 to triggers a single i915->audio notify
> > > call after the audio driver has finished its probe. Or we could do
> > > whatever we seem to have now is where the audio driver can just root
> > > around directly in the structure (at which point passing any
> > > parameters in the notify calls seems redundant as well).
> >
> > Looking at some older emails, i think you recommended a 'register'
> > callback to let the audio driver signal to the i915 side it completed
> > its initialization, with a single notify generated if needed (what you
> > described just above as 'the other way')
> >
> > If you look at the path 4 of the series,
> 
> I seem to have missed the whole series by some reason, both to my inbox
> and to ML.  Could you resubmit later?
> 

Yes - I will resend rfc v4 after addressing some of Ville's comment.

> 
> > Jerome was trying to
> > implement this with a 'notify_pending' field in the platform data set
> > by the i915 side and used during the audio driver probe
> >

Yes - thanks Pierre. I believe Ville's comment was addressed with this change. Am not sure if there is anything critically
missed out leading to a flaw

> > +	if (pdata->notify_pending) {
> > +
> > +		pr_debug("%s: handle pending notification\n", __func__);
> > +		notify_audio_lpe(&pdata->eld);
> > +		pdata->notify_pending = false;
> > +	}
> >
> > Maybe an explicit handshake is more self-explanatory and safer?
> 
> IMO yes, the pending notification flag isn't intrusive.
> 

Yes - I thought it was simpler to handle the notification in this manner

> There are cases where the audio driver wants to inquire the status from gfx
> side; at least, the HD-audio driver needed to recheck after PM.
> 

Am not sure if there is any case like that to be handled here. Hence a simpler interface was created and implemented

> Also, isn't the code above racy without a proper mutex?
> 

No

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