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

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

 




> -----Original Message-----
> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@xxxxxxxxxxxxxxx]
> Sent: Saturday, January 7, 2017 1:43 AM
> To: Anand, Jerome <jerome.anand@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx
> Cc: tiwai@xxxxxxx; broonie@xxxxxxxxxx; Ughreja, Rakesh A
> <rakesh.a.ughreja@xxxxxxxxx>; ville.syrjala@xxxxxxxxxxxxxxx
> Subject: Re: [alsa-devel] [PATCH V2 1/7] drm/i915: setup bridge for HDMI LPE
> audio driver
> 
> Thanks for the update Jerome. Looks like you missed a couple of agreed
> comments from the last round?
> 

Thanks Pierre for the quick feedback. I though I replied to some of the 
Comments previously.

> On 1/6/17 7:21 PM, 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jerome Anand <jerome.anand@xxxxxxxxx>
> > ---
> >  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        |  16 ++
> >  drivers/gpu/drm/i915/i915_reg.h        |   3 +
> >  drivers/gpu/drm/i915/intel_lpe_audio.c | 355
> +++++++++++++++++++++++++++++++++
> >  include/drm/intel_lpe_audio.h          |  45 +++++
> >  8 files changed, 452 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 104296d..bd9b767 100644
> > --- a/Documentation/gpu/i915.rst
> > +++ b/Documentation/gpu/i915.rst
> > @@ -225,6 +225,15 @@ Display PLLs
> >  .. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.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 5196509..2bca239 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -127,6 +127,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 4d22b4b..70d728b 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1131,7 +1131,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 @@ -1149,7 +1150,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 7b43662..2f8165e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2460,6 +2460,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.
> > @@ -2859,6 +2865,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
> > @@ -3620,6 +3628,13 @@ extern int i915_restore_state(struct
> > drm_i915_private *dev_priv);  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_i915_private *dev_priv);
> > extern void intel_teardown_gmbus(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c index a0e70f5..d9393d6a 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1893,6 +1893,10 @@ static irqreturn_t valleyview_irq_handler(int irq,
> void *arg)
> >  		 * signalled in iir */
> >  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> >
> > +		if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> > +			   I915_LPE_PIPE_B_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.
> > @@ -1973,6 +1977,11 @@ static irqreturn_t cherryview_irq_handler(int irq,
> void *arg)
> >  		 * signalled in iir */
> >  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> >
> > +		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.
> > @@ -2914,6 +2923,7 @@ static void vlv_display_irq_postinstall(struct
> drm_i915_private *dev_priv)
> >  	u32 pipestat_mask;
> >  	u32 enable_mask;
> >  	enum pipe pipe;
> > +	u32 val;
> >
> >  	pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
> >  			PIPE_CRC_DONE_INTERRUPT_STATUS;
> > @@ -2930,6 +2940,12 @@ static void vlv_display_irq_postinstall(struct
> > drm_i915_private *dev_priv)
> >
> >  	WARN_ON(dev_priv->irq_mask != ~0);
> >
> > +	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 00970aa..643bc6e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2428,6 +2428,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..05f5e4e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > @@ -0,0 +1,355 @@
> > +/*
> > + * 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@xxxxxxxxxxxxxxx>
> > + *    Jerome Anand <jerome.anand@xxxxxxxxx>
> > + *    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);
> 
> from Takashi: This was already tested, no?
> JA: OK - can be removed
> 

Sorry missed it.

> > +
> > +	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);
> > +}
> 
> 
> from Takashi:
>   guess using platform_device_register_full() makes the code a bit simpler.
> JA: OK - agreed, but will keep it since its acked by Daniel.
> -> what's the downside to using simpler code?
> 

I felt the existing implementation is also simple and would like to keep it. 
I don’t see any advantage with the proposed change.

> 
> > +
> > +static void lpe_audio_platdev_destroy(struct drm_i915_private
> > +*dev_priv) {
> > +	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_i915_private *dev_priv = d->chip_data;
> > +	unsigned long irqflags;
> > +	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> > +		I915_LPE_PIPE_B_INTERRUPT);
> 
>  From Daniel: PIPE_C missing?
> -> Yes for CherryTrail
> 

This was asked by Ville to remove it since there is no PIPE C in VLV.
So I believe it shouldn't make any difference

> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > +	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_i915_private *dev_priv = d->chip_data;
> > +	unsigned long irqflags;
> > +	u32 val = (I915_LPE_PIPE_A_INTERRUPT |
> > +		I915_LPE_PIPE_B_INTERRUPT);
> 
>  From Daniel: PIPE_C missing?
> -> Yes for CherryTrail
> 

This was asked by Ville to remove it since there is no PIPE C in VLV.
So I believe it shouldn't make any difference

> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > +	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); }
> > +
> > +/**
> > + * 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;
> > +
> > +	if (!HAS_LPE_AUDIO(dev_priv))
> > +		return;
> > +
> > +	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)) {
> > +		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)},
> > +			{}
> > +		};
> > +
> > +		if (!pci_dev_present(atom_hdaudio_ids)) {
> > +			DRM_INFO("%s%s\n", "HDaudio controller not
> detected,",
> > +				"using LPE audio instead\n");
> 
>  From Takashi:
> Why %s%s?  Just keep one line without line breakage.
> The 80 chars rule is just a suggestion, no strict rule at all.
> JA: OK
> 

Logging should be fine. I prefer to keep this.

> 
> > +			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;
> > +
> > +	if (!HAS_LPE_AUDIO(dev_priv))
> > +		return;
> > +
> > +	desc = irq_to_desc(dev_priv->lpe_audio.irq);
> > +
> > +	lpe_audio_irq_mask(&desc->irq_data);
> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > +	lpe_audio_platdev_destroy(dev_priv);
> > +
> > +	irq_free_desc(dev_priv->lpe_audio.irq);
> > +
> > +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> 
>  From Takashi: What's the reason of this spinlock?
> JA: JIC - probably not needed now
> 

Kept this for the irq free.
Will remove it and test once

> > +}
> > 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_ */
> >

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




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