diff mbox

[RFC,v2,1/8] drm/i915: setup bridge for HDMI LPE audio driver

Message ID 20161001002242.31025-2-jerome.anand@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jerome Anand Oct. 1, 2016, 12:22 a.m. UTC
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>
---
 drivers/gpu/drm/i915/Makefile          |   3 +
 drivers/gpu/drm/i915/i915_drv.c        |  13 +-
 drivers/gpu/drm/i915/i915_drv.h        |  19 ++
 drivers/gpu/drm/i915/i915_irq.c        |  14 ++
 drivers/gpu/drm/i915/i915_reg.h        |   3 +
 drivers/gpu/drm/i915/intel_lpe_audio.c | 357 +++++++++++++++++++++++++++++++++
 include/drm/intel_lpe_audio.h          |  45 +++++
 7 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

Comments

Pierre-Louis Bossart Oct. 13, 2016, 7:38 p.m. UTC | #1
Thanks Ville for the review. A lot of the comments are related to the 
initial VED code we took pretty much as is, no issues to clean-up further.

BTW, it looks like Jerome's patches were stuck for 10+ days on the 
intel-gfx server for some reason so not everyone saw the initial post?

>> @@ -1141,7 +1141,13 @@ 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_detect(dev_priv)) {
>> +		if (intel_lpe_audio_setup(dev_priv) < 0)
>> +			DRM_ERROR("failed to setup LPE Audio bridge\n");
>> +	}
>
> I'd move all that into the lpe audio code. No need to have anything here
> but a single function call IMO.

something like intel_lpe_audio_init() for symmetry with the hda 
component stuff, with both detection and setup handled internally?
>
>> +
>> +	if (!IS_LPE_AUDIO_ENABLED(dev_priv))
>
> I don't like that too much. I think I would just make
> that HAS_LPE_AUDIO(). The current IS_VLV||IS_CHV check can just be
> inlined into the init function.

ok


>>
>>  #define HAS_POOLED_EU(dev)	(INTEL_INFO(dev)->has_pooled_eu)
>>
>> +#define HAS_LPE_AUDIO(dev)	(IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
>> +#define IS_LPE_AUDIO_ENABLED(dev_priv) \
>> +				(__I915__(dev_priv)->lpe_audio.platdev != NULL)
>> +#define IS_LPE_AUDIO_IRQ_VALID(dev_priv) \
>> +				(__I915__(dev_priv)->lpe_audio.irq >= 0)
>
> Seems useless. We should just not register the lpe audio thing if we
> have no irq.

ok

>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1827,6 +1827,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>>  		 * signalled in iir */
>>  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>>
>> +		if (IS_LPE_AUDIO_ENABLED(dev_priv))
>> +			if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
>
> I think both of these checks can be removed. We won't unmask the
> interrupts unless lpe is enabled, so the IIR bits will never be set in
> that case.
>
>> +				if (iir & (I915_LPE_PIPE_A_INTERRUPT |
>> +					I915_LPE_PIPE_B_INTERRUPT |
>> +					I915_LPE_PIPE_C_INTERRUPT))
>> +					intel_lpe_audio_irq_handler(dev_priv);
>> +

ok.

>>  		/*
>>  		 * VLV_IIR is single buffered, and reflects the level
>>  		 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
>> @@ -1907,6 +1914,13 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>>  		 * signalled in iir */
>>  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>>
>> +		if (IS_LPE_AUDIO_ENABLED(dev_priv))
>> +			if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
>> +				if (iir & (I915_LPE_PIPE_A_INTERRUPT |
>> +					I915_LPE_PIPE_B_INTERRUPT |
>> +					I915_LPE_PIPE_C_INTERRUPT))
>> +					intel_lpe_audio_irq_handler(dev_priv);
>> +
>
> ditto

ok


>> +	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() */
>
> What's this about?

Dunno, this was in the original VED series that we used as a baseline. 
Unless someone has memories from that time, i'll just remove this comment.


>> +	DRM_DEBUG("%s: HDMI_AUDIO rsc.start[0] = 0x%x\n",
>> +		__func__, (int)(rsc[0].start));
>
> __func__ pointless since DRM_DEBUG alreay adds it. Also saying
> "rsc.start[0]" in the message doesn't tell anyone anything useful.
> And I think irq numbers are usually printed in decimal.

Same, this was taken from the VED series, no issue to clean-up.

>
>> +
>> +	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";
>> +
>> +	DRM_DEBUG("%s: HDMI_AUDIO rsc.start[1] = 0x%x\n",
>> +		__func__, (int)(rsc[1].start));
>
> Again "rsc[0].start" doesn't seem like anything useful to print in a
> debug message. Don't see much point in the whole debug message TBH since
> the start/end are fixed.

so are you saying we should remove the code or move the level to 
DRM_INFO - for extra verbose debug?

>
>> +
>> +	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;
>> +
>> +	/* 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;
>> +	}
>> +	spin_lock_init(&pdata->lpe_audio_slock);
>
> Should init it before adding the device I suppose? It's not used in the
> patch so hard to say. In general the patch split is not the best due to
> not being able to see the use of things.

ok. we'll look into this.


>> +static void lpe_audio_platdev_destroy(struct drm_i915_private *dev_priv)
>> +{
>> +	if (dev_priv->lpe_audio.platdev) {
>> +		platform_device_unregister(dev_priv->lpe_audio.platdev);
>> +		kfree(dev_priv->lpe_audio.platdev->dev.dma_mask);
>> +	}
>
> I usually prefer
>
> {
> 	if (!stuff)
> 		return;
>
> 	other stuff;
> }
>
> just to keep the indentation better in check.

ok

>
>> +}
>> +
>> +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);
>> +
>> +	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);
>> +
>
> Extra newline here for some reason. No such thing in the counterpart.

ok


>> +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));
>
> Could leave a blank like here to make things look less convoluted.

ok

>
>> +	irq_set_chip_and_handler_name(irq,
>> +		&lpe_audio_irqchip,
>> +		handle_simple_irq,
>> +		"hdmi_lpe_audio_irq_handler");
>
> Indentation isn't quite right.

ok

>
>> +
>> +	return irq_set_chip_data(irq, &dev_priv->drm);
>> +}
>> +
>> +/**
>> + * 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
>> + */
>> +int intel_lpe_audio_detect(struct drm_i915_private *dev_priv)
>> +{
>> +	int lpe_present = false;
>> +	struct drm_device *dev = &dev_priv->drm;
>> +
>> +	if (HAS_LPE_AUDIO(dev)) {
>> +		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)},
>> +			{}
>> +		};
>
> Hmm. Maybe I asked already, but could we use a class match instead?
> There's some kind of HDA class right?

I am not aware of any class, this is really the only means we found to 
test if the HDaudio controller is disabled or fused out. Maybe a 
question for Takashi?

>
>> +
>> +		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_descs(-1, 0, 1, 0);
>
> aka. irq_alloc_desc() ?

not sure, will look into this.

>
>> +	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("%s : irq = %d\n", __func__, dev_priv->lpe_audio.irq);
>
> Another __func__ that's not needed. And we're printing the irq twice
> now?

ok

>
>> +
>> +	ret = lpe_audio_irq_init(dev_priv);
>> +
>> +	if (ret) {
>> +
>> +		DRM_ERROR("Failed to initialize irqchip for lpe audio: %d\n",
>> +			ret);
>
> Indentation looks off.

ok
>
>> +		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);
>
> ditto

ok

>> +void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
>> +{
>> +	unsigned long irqflags;
>> +	struct irq_desc *desc;
>> +
>> +	desc = IS_LPE_AUDIO_IRQ_VALID(dev_priv) ?
>> +			irq_to_desc(dev_priv->lpe_audio.irq) : NULL;
>> +
>> +	/**
>> +	 * mask LPE audio irq before destroying
>> +	 */
>> +	if (desc)
>
> Seems overly defensive. Should just check if we have the platform device
> at the start, and otherwise we can assume that all the things we need to
> free are there.
>
> This looks racy too. We should unregister the platform device as the
> first thing, and only then free up the resources and whatnot.

will clean up
Ville Syrjälä Oct. 14, 2016, 9:10 a.m. UTC | #2
On Thu, Oct 13, 2016 at 02:38:30PM -0500, Pierre-Louis Bossart wrote:
> Thanks Ville for the review. A lot of the comments are related to the 
> initial VED code we took pretty much as is, no issues to clean-up further.
> 
> BTW, it looks like Jerome's patches were stuck for 10+ days on the 
> intel-gfx server for some reason so not everyone saw the initial post?

Did they make it to the list? Jani told me they didn't, nor were they in
the moderation queue apparently. So no idea where they went. I tried
bouncing them to the list, but I'm not sure they came through that time
either.

> 
> >> @@ -1141,7 +1141,13 @@ 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_detect(dev_priv)) {
> >> +		if (intel_lpe_audio_setup(dev_priv) < 0)
> >> +			DRM_ERROR("failed to setup LPE Audio bridge\n");
> >> +	}
> >
> > I'd move all that into the lpe audio code. No need to have anything here
> > but a single function call IMO.
> 
> something like intel_lpe_audio_init() for symmetry with the hda 
> component stuff, with both detection and setup handled internally?
> >
> >> +
> >> +	if (!IS_LPE_AUDIO_ENABLED(dev_priv))
> >
> > I don't like that too much. I think I would just make
> > that HAS_LPE_AUDIO(). The current IS_VLV||IS_CHV check can just be
> > inlined into the init function.
> 
> ok
> 
> 
> >>
> >>  #define HAS_POOLED_EU(dev)	(INTEL_INFO(dev)->has_pooled_eu)
> >>
> >> +#define HAS_LPE_AUDIO(dev)	(IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> >> +#define IS_LPE_AUDIO_ENABLED(dev_priv) \
> >> +				(__I915__(dev_priv)->lpe_audio.platdev != NULL)
> >> +#define IS_LPE_AUDIO_IRQ_VALID(dev_priv) \
> >> +				(__I915__(dev_priv)->lpe_audio.irq >= 0)
> >
> > Seems useless. We should just not register the lpe audio thing if we
> > have no irq.
> 
> ok
> 
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -1827,6 +1827,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> >>  		 * signalled in iir */
> >>  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> >>
> >> +		if (IS_LPE_AUDIO_ENABLED(dev_priv))
> >> +			if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
> >
> > I think both of these checks can be removed. We won't unmask the
> > interrupts unless lpe is enabled, so the IIR bits will never be set in
> > that case.
> >
> >> +				if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> >> +					I915_LPE_PIPE_B_INTERRUPT |
> >> +					I915_LPE_PIPE_C_INTERRUPT))
> >> +					intel_lpe_audio_irq_handler(dev_priv);
> >> +
> 
> ok.
> 
> >>  		/*
> >>  		 * VLV_IIR is single buffered, and reflects the level
> >>  		 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
> >> @@ -1907,6 +1914,13 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
> >>  		 * signalled in iir */
> >>  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> >>
> >> +		if (IS_LPE_AUDIO_ENABLED(dev_priv))
> >> +			if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
> >> +				if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> >> +					I915_LPE_PIPE_B_INTERRUPT |
> >> +					I915_LPE_PIPE_C_INTERRUPT))
> >> +					intel_lpe_audio_irq_handler(dev_priv);
> >> +
> >
> > ditto
> 
> ok
> 
> 
> >> +	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() */
> >
> > What's this about?
> 
> Dunno, this was in the original VED series that we used as a baseline. 
> Unless someone has memories from that time, i'll just remove this comment.
> 
> 
> >> +	DRM_DEBUG("%s: HDMI_AUDIO rsc.start[0] = 0x%x\n",
> >> +		__func__, (int)(rsc[0].start));
> >
> > __func__ pointless since DRM_DEBUG alreay adds it. Also saying
> > "rsc.start[0]" in the message doesn't tell anyone anything useful.
> > And I think irq numbers are usually printed in decimal.
> 
> Same, this was taken from the VED series, no issue to clean-up.
> 
> >
> >> +
> >> +	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";
> >> +
> >> +	DRM_DEBUG("%s: HDMI_AUDIO rsc.start[1] = 0x%x\n",
> >> +		__func__, (int)(rsc[1].start));
> >
> > Again "rsc[0].start" doesn't seem like anything useful to print in a
> > debug message. Don't see much point in the whole debug message TBH since
> > the start/end are fixed.
> 
> so are you saying we should remove the code or move the level to 
> DRM_INFO - for extra verbose debug?
> 
> >
> >> +
> >> +	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;
> >> +
> >> +	/* 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;
> >> +	}
> >> +	spin_lock_init(&pdata->lpe_audio_slock);
> >
> > Should init it before adding the device I suppose? It's not used in the
> > patch so hard to say. In general the patch split is not the best due to
> > not being able to see the use of things.
> 
> ok. we'll look into this.
> 
> 
> >> +static void lpe_audio_platdev_destroy(struct drm_i915_private *dev_priv)
> >> +{
> >> +	if (dev_priv->lpe_audio.platdev) {
> >> +		platform_device_unregister(dev_priv->lpe_audio.platdev);
> >> +		kfree(dev_priv->lpe_audio.platdev->dev.dma_mask);
> >> +	}
> >
> > I usually prefer
> >
> > {
> > 	if (!stuff)
> > 		return;
> >
> > 	other stuff;
> > }
> >
> > just to keep the indentation better in check.
> 
> ok
> 
> >
> >> +}
> >> +
> >> +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);
> >> +
> >> +	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);
> >> +
> >
> > Extra newline here for some reason. No such thing in the counterpart.
> 
> ok
> 
> 
> >> +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));
> >
> > Could leave a blank like here to make things look less convoluted.
> 
> ok
> 
> >
> >> +	irq_set_chip_and_handler_name(irq,
> >> +		&lpe_audio_irqchip,
> >> +		handle_simple_irq,
> >> +		"hdmi_lpe_audio_irq_handler");
> >
> > Indentation isn't quite right.
> 
> ok
> 
> >
> >> +
> >> +	return irq_set_chip_data(irq, &dev_priv->drm);
> >> +}
> >> +
> >> +/**
> >> + * 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
> >> + */
> >> +int intel_lpe_audio_detect(struct drm_i915_private *dev_priv)
> >> +{
> >> +	int lpe_present = false;
> >> +	struct drm_device *dev = &dev_priv->drm;
> >> +
> >> +	if (HAS_LPE_AUDIO(dev)) {
> >> +		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)},
> >> +			{}
> >> +		};
> >
> > Hmm. Maybe I asked already, but could we use a class match instead?
> > There's some kind of HDA class right?
> 
> I am not aware of any class, this is really the only means we found to 
> test if the HDaudio controller is disabled or fused out. Maybe a 
> question for Takashi?
> 
> >
> >> +
> >> +		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_descs(-1, 0, 1, 0);
> >
> > aka. irq_alloc_desc() ?
> 
> not sure, will look into this.
> 
> >
> >> +	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("%s : irq = %d\n", __func__, dev_priv->lpe_audio.irq);
> >
> > Another __func__ that's not needed. And we're printing the irq twice
> > now?
> 
> ok
> 
> >
> >> +
> >> +	ret = lpe_audio_irq_init(dev_priv);
> >> +
> >> +	if (ret) {
> >> +
> >> +		DRM_ERROR("Failed to initialize irqchip for lpe audio: %d\n",
> >> +			ret);
> >
> > Indentation looks off.
> 
> ok
> >
> >> +		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);
> >
> > ditto
> 
> ok
> 
> >> +void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
> >> +{
> >> +	unsigned long irqflags;
> >> +	struct irq_desc *desc;
> >> +
> >> +	desc = IS_LPE_AUDIO_IRQ_VALID(dev_priv) ?
> >> +			irq_to_desc(dev_priv->lpe_audio.irq) : NULL;
> >> +
> >> +	/**
> >> +	 * mask LPE audio irq before destroying
> >> +	 */
> >> +	if (desc)
> >
> > Seems overly defensive. Should just check if we have the platform device
> > at the start, and otherwise we can assume that all the things we need to
> > free are there.
> >
> > This looks racy too. We should unregister the platform device as the
> > first thing, and only then free up the resources and whatnot.
> 
> will clean up
>
Jani Nikula Oct. 14, 2016, 1:50 p.m. UTC | #3
On Fri, 14 Oct 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Oct 13, 2016 at 02:38:30PM -0500, Pierre-Louis Bossart wrote:
>> Thanks Ville for the review. A lot of the comments are related to the 
>> initial VED code we took pretty much as is, no issues to clean-up further.
>> 
>> BTW, it looks like Jerome's patches were stuck for 10+ days on the 
>> intel-gfx server for some reason so not everyone saw the initial post?
>
> Did they make it to the list? Jani told me they didn't, nor were they in
> the moderation queue apparently. So no idea where they went. I tried
> bouncing them to the list, but I'm not sure they came through that time
> either.

The patches as bounced by Ville made it to the list, but the original
ones were lost and I have no idea what happened. They were not in
moderation and there was no trace of them.

BR,
Jani.



>
>> 
>> >> @@ -1141,7 +1141,13 @@ 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_detect(dev_priv)) {
>> >> +		if (intel_lpe_audio_setup(dev_priv) < 0)
>> >> +			DRM_ERROR("failed to setup LPE Audio bridge\n");
>> >> +	}
>> >
>> > I'd move all that into the lpe audio code. No need to have anything here
>> > but a single function call IMO.
>> 
>> something like intel_lpe_audio_init() for symmetry with the hda 
>> component stuff, with both detection and setup handled internally?
>> >
>> >> +
>> >> +	if (!IS_LPE_AUDIO_ENABLED(dev_priv))
>> >
>> > I don't like that too much. I think I would just make
>> > that HAS_LPE_AUDIO(). The current IS_VLV||IS_CHV check can just be
>> > inlined into the init function.
>> 
>> ok
>> 
>> 
>> >>
>> >>  #define HAS_POOLED_EU(dev)	(INTEL_INFO(dev)->has_pooled_eu)
>> >>
>> >> +#define HAS_LPE_AUDIO(dev)	(IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
>> >> +#define IS_LPE_AUDIO_ENABLED(dev_priv) \
>> >> +				(__I915__(dev_priv)->lpe_audio.platdev != NULL)
>> >> +#define IS_LPE_AUDIO_IRQ_VALID(dev_priv) \
>> >> +				(__I915__(dev_priv)->lpe_audio.irq >= 0)
>> >
>> > Seems useless. We should just not register the lpe audio thing if we
>> > have no irq.
>> 
>> ok
>> 
>> >> --- a/drivers/gpu/drm/i915/i915_irq.c
>> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> >> @@ -1827,6 +1827,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>> >>  		 * signalled in iir */
>> >>  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>> >>
>> >> +		if (IS_LPE_AUDIO_ENABLED(dev_priv))
>> >> +			if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
>> >
>> > I think both of these checks can be removed. We won't unmask the
>> > interrupts unless lpe is enabled, so the IIR bits will never be set in
>> > that case.
>> >
>> >> +				if (iir & (I915_LPE_PIPE_A_INTERRUPT |
>> >> +					I915_LPE_PIPE_B_INTERRUPT |
>> >> +					I915_LPE_PIPE_C_INTERRUPT))
>> >> +					intel_lpe_audio_irq_handler(dev_priv);
>> >> +
>> 
>> ok.
>> 
>> >>  		/*
>> >>  		 * VLV_IIR is single buffered, and reflects the level
>> >>  		 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
>> >> @@ -1907,6 +1914,13 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>> >>  		 * signalled in iir */
>> >>  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>> >>
>> >> +		if (IS_LPE_AUDIO_ENABLED(dev_priv))
>> >> +			if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
>> >> +				if (iir & (I915_LPE_PIPE_A_INTERRUPT |
>> >> +					I915_LPE_PIPE_B_INTERRUPT |
>> >> +					I915_LPE_PIPE_C_INTERRUPT))
>> >> +					intel_lpe_audio_irq_handler(dev_priv);
>> >> +
>> >
>> > ditto
>> 
>> ok
>> 
>> 
>> >> +	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() */
>> >
>> > What's this about?
>> 
>> Dunno, this was in the original VED series that we used as a baseline. 
>> Unless someone has memories from that time, i'll just remove this comment.
>> 
>> 
>> >> +	DRM_DEBUG("%s: HDMI_AUDIO rsc.start[0] = 0x%x\n",
>> >> +		__func__, (int)(rsc[0].start));
>> >
>> > __func__ pointless since DRM_DEBUG alreay adds it. Also saying
>> > "rsc.start[0]" in the message doesn't tell anyone anything useful.
>> > And I think irq numbers are usually printed in decimal.
>> 
>> Same, this was taken from the VED series, no issue to clean-up.
>> 
>> >
>> >> +
>> >> +	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";
>> >> +
>> >> +	DRM_DEBUG("%s: HDMI_AUDIO rsc.start[1] = 0x%x\n",
>> >> +		__func__, (int)(rsc[1].start));
>> >
>> > Again "rsc[0].start" doesn't seem like anything useful to print in a
>> > debug message. Don't see much point in the whole debug message TBH since
>> > the start/end are fixed.
>> 
>> so are you saying we should remove the code or move the level to 
>> DRM_INFO - for extra verbose debug?
>> 
>> >
>> >> +
>> >> +	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;
>> >> +
>> >> +	/* 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;
>> >> +	}
>> >> +	spin_lock_init(&pdata->lpe_audio_slock);
>> >
>> > Should init it before adding the device I suppose? It's not used in the
>> > patch so hard to say. In general the patch split is not the best due to
>> > not being able to see the use of things.
>> 
>> ok. we'll look into this.
>> 
>> 
>> >> +static void lpe_audio_platdev_destroy(struct drm_i915_private *dev_priv)
>> >> +{
>> >> +	if (dev_priv->lpe_audio.platdev) {
>> >> +		platform_device_unregister(dev_priv->lpe_audio.platdev);
>> >> +		kfree(dev_priv->lpe_audio.platdev->dev.dma_mask);
>> >> +	}
>> >
>> > I usually prefer
>> >
>> > {
>> > 	if (!stuff)
>> > 		return;
>> >
>> > 	other stuff;
>> > }
>> >
>> > just to keep the indentation better in check.
>> 
>> ok
>> 
>> >
>> >> +}
>> >> +
>> >> +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);
>> >> +
>> >> +	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);
>> >> +
>> >
>> > Extra newline here for some reason. No such thing in the counterpart.
>> 
>> ok
>> 
>> 
>> >> +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));
>> >
>> > Could leave a blank like here to make things look less convoluted.
>> 
>> ok
>> 
>> >
>> >> +	irq_set_chip_and_handler_name(irq,
>> >> +		&lpe_audio_irqchip,
>> >> +		handle_simple_irq,
>> >> +		"hdmi_lpe_audio_irq_handler");
>> >
>> > Indentation isn't quite right.
>> 
>> ok
>> 
>> >
>> >> +
>> >> +	return irq_set_chip_data(irq, &dev_priv->drm);
>> >> +}
>> >> +
>> >> +/**
>> >> + * 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
>> >> + */
>> >> +int intel_lpe_audio_detect(struct drm_i915_private *dev_priv)
>> >> +{
>> >> +	int lpe_present = false;
>> >> +	struct drm_device *dev = &dev_priv->drm;
>> >> +
>> >> +	if (HAS_LPE_AUDIO(dev)) {
>> >> +		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)},
>> >> +			{}
>> >> +		};
>> >
>> > Hmm. Maybe I asked already, but could we use a class match instead?
>> > There's some kind of HDA class right?
>> 
>> I am not aware of any class, this is really the only means we found to 
>> test if the HDaudio controller is disabled or fused out. Maybe a 
>> question for Takashi?
>> 
>> >
>> >> +
>> >> +		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_descs(-1, 0, 1, 0);
>> >
>> > aka. irq_alloc_desc() ?
>> 
>> not sure, will look into this.
>> 
>> >
>> >> +	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("%s : irq = %d\n", __func__, dev_priv->lpe_audio.irq);
>> >
>> > Another __func__ that's not needed. And we're printing the irq twice
>> > now?
>> 
>> ok
>> 
>> >
>> >> +
>> >> +	ret = lpe_audio_irq_init(dev_priv);
>> >> +
>> >> +	if (ret) {
>> >> +
>> >> +		DRM_ERROR("Failed to initialize irqchip for lpe audio: %d\n",
>> >> +			ret);
>> >
>> > Indentation looks off.
>> 
>> ok
>> >
>> >> +		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);
>> >
>> > ditto
>> 
>> ok
>> 
>> >> +void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
>> >> +{
>> >> +	unsigned long irqflags;
>> >> +	struct irq_desc *desc;
>> >> +
>> >> +	desc = IS_LPE_AUDIO_IRQ_VALID(dev_priv) ?
>> >> +			irq_to_desc(dev_priv->lpe_audio.irq) : NULL;
>> >> +
>> >> +	/**
>> >> +	 * mask LPE audio irq before destroying
>> >> +	 */
>> >> +	if (desc)
>> >
>> > Seems overly defensive. Should just check if we have the platform device
>> > at the start, and otherwise we can assume that all the things we need to
>> > free are there.
>> >
>> > This looks racy too. We should unregister the platform device as the
>> > first thing, and only then free up the resources and whatnot.
>> 
>> will clean up
>>
Daniel Vetter Oct. 17, 2016, 6:46 a.m. UTC | #4
On Sat, Oct 01, 2016 at 05:52:35AM +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>

You need to add an include-stanza to Documentation/gpu/i915.rst,
otherwise the kerneldoc won't show up. Please also do some docs for the
structures, and then run

$ make htmldocs

to check the output looks reasonable.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/Makefile          |   3 +
>  drivers/gpu/drm/i915/i915_drv.c        |  13 +-
>  drivers/gpu/drm/i915/i915_drv.h        |  19 ++
>  drivers/gpu/drm/i915/i915_irq.c        |  14 ++
>  drivers/gpu/drm/i915/i915_reg.h        |   3 +
>  drivers/gpu/drm/i915/intel_lpe_audio.c | 357 +++++++++++++++++++++++++++++++++
>  include/drm/intel_lpe_audio.h          |  45 +++++
>  7 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/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e6fe004..11f9741 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -115,6 +115,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 31b2b63..ab1e4768 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1141,7 +1141,13 @@ 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_detect(dev_priv)) {
> +		if (intel_lpe_audio_setup(dev_priv) < 0)
> +			DRM_ERROR("failed to setup LPE Audio bridge\n");
> +	}
> +
> +	if (!IS_LPE_AUDIO_ENABLED(dev_priv))
> +		i915_audio_component_init(dev_priv);
>  
>  	/*
>  	 * Some ports require correctly set-up hpd registers for detection to
> @@ -1159,7 +1165,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 (IS_LPE_AUDIO_ENABLED(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 91ff3d7..399a8ee 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2087,6 +2087,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.
> @@ -2827,6 +2833,13 @@ struct drm_i915_cmd_table {
>  
>  #define HAS_POOLED_EU(dev)	(INTEL_INFO(dev)->has_pooled_eu)
>  
> +#define HAS_LPE_AUDIO(dev)	(IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> +#define IS_LPE_AUDIO_ENABLED(dev_priv) \
> +				(__I915__(dev_priv)->lpe_audio.platdev != NULL)
> +#define IS_LPE_AUDIO_IRQ_VALID(dev_priv) \
> +				(__I915__(dev_priv)->lpe_audio.irq >= 0)
> +
> +
>  #define INTEL_PCH_DEVICE_ID_MASK		0xff00
>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
>  #define INTEL_PCH_CPT_DEVICE_ID_TYPE		0x1c00
> @@ -3579,6 +3592,12 @@ 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_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);
> +int 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 6ed5b24..d8f515f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1827,6 +1827,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  		 * signalled in iir */
>  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>  
> +		if (IS_LPE_AUDIO_ENABLED(dev_priv))
> +			if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
> +				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.
> @@ -1907,6 +1914,13 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>  		 * signalled in iir */
>  		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>  
> +		if (IS_LPE_AUDIO_ENABLED(dev_priv))
> +			if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
> +				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.
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8d44cee..fb734f2 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2137,6 +2137,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..acfe22f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> @@ -0,0 +1,357 @@
> +/*
> + * 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 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";
> +
> +	DRM_DEBUG("%s: HDMI_AUDIO rsc.start[0] = 0x%x\n",
> +		__func__, (int)(rsc[0].start));
> +
> +	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";
> +
> +	DRM_DEBUG("%s: HDMI_AUDIO rsc.start[1] = 0x%x\n",
> +		__func__, (int)(rsc[1].start));
> +
> +	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;
> +
> +	/* 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;
> +	}
> +	spin_lock_init(&pdata->lpe_audio_slock);
> +
> +	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 (dev_priv->lpe_audio.platdev) {
> +		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);
> +
> +	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);
> +
> +	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);
> +}
> +
> +/**
> + * 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
> + */
> +int intel_lpe_audio_detect(struct drm_i915_private *dev_priv)
> +{
> +	int lpe_present = false;
> +	struct drm_device *dev = &dev_priv->drm;
> +
> +	if (HAS_LPE_AUDIO(dev)) {
> +		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");
> +			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_descs(-1, 0, 1, 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("%s : irq = %d\n", __func__, 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_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 = IS_LPE_AUDIO_IRQ_VALID(dev_priv) ?
> +			irq_to_desc(dev_priv->lpe_audio.irq) : NULL;
> +
> +	/**
> +	 * mask LPE audio irq before destroying
> +	 */
> +	if (desc)
> +		lpe_audio_irq_mask(&desc->irq_data);
> +
> +	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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index e6fe004..11f9741 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -115,6 +115,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 31b2b63..ab1e4768 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1141,7 +1141,13 @@  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_detect(dev_priv)) {
+		if (intel_lpe_audio_setup(dev_priv) < 0)
+			DRM_ERROR("failed to setup LPE Audio bridge\n");
+	}
+
+	if (!IS_LPE_AUDIO_ENABLED(dev_priv))
+		i915_audio_component_init(dev_priv);
 
 	/*
 	 * Some ports require correctly set-up hpd registers for detection to
@@ -1159,7 +1165,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 (IS_LPE_AUDIO_ENABLED(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 91ff3d7..399a8ee 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2087,6 +2087,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.
@@ -2827,6 +2833,13 @@  struct drm_i915_cmd_table {
 
 #define HAS_POOLED_EU(dev)	(INTEL_INFO(dev)->has_pooled_eu)
 
+#define HAS_LPE_AUDIO(dev)	(IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
+#define IS_LPE_AUDIO_ENABLED(dev_priv) \
+				(__I915__(dev_priv)->lpe_audio.platdev != NULL)
+#define IS_LPE_AUDIO_IRQ_VALID(dev_priv) \
+				(__I915__(dev_priv)->lpe_audio.irq >= 0)
+
+
 #define INTEL_PCH_DEVICE_ID_MASK		0xff00
 #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
 #define INTEL_PCH_CPT_DEVICE_ID_TYPE		0x1c00
@@ -3579,6 +3592,12 @@  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_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);
+int 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 6ed5b24..d8f515f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1827,6 +1827,13 @@  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 		 * signalled in iir */
 		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
 
+		if (IS_LPE_AUDIO_ENABLED(dev_priv))
+			if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
+				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.
@@ -1907,6 +1914,13 @@  static irqreturn_t cherryview_irq_handler(int irq, void *arg)
 		 * signalled in iir */
 		valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
 
+		if (IS_LPE_AUDIO_ENABLED(dev_priv))
+			if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
+				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.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8d44cee..fb734f2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2137,6 +2137,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..acfe22f
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
@@ -0,0 +1,357 @@ 
+/*
+ * 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 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";
+
+	DRM_DEBUG("%s: HDMI_AUDIO rsc.start[0] = 0x%x\n",
+		__func__, (int)(rsc[0].start));
+
+	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";
+
+	DRM_DEBUG("%s: HDMI_AUDIO rsc.start[1] = 0x%x\n",
+		__func__, (int)(rsc[1].start));
+
+	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;
+
+	/* 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;
+	}
+	spin_lock_init(&pdata->lpe_audio_slock);
+
+	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 (dev_priv->lpe_audio.platdev) {
+		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);
+
+	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);
+
+	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);
+}
+
+/**
+ * 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
+ */
+int intel_lpe_audio_detect(struct drm_i915_private *dev_priv)
+{
+	int lpe_present = false;
+	struct drm_device *dev = &dev_priv->drm;
+
+	if (HAS_LPE_AUDIO(dev)) {
+		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");
+			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_descs(-1, 0, 1, 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("%s : irq = %d\n", __func__, 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_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 = IS_LPE_AUDIO_IRQ_VALID(dev_priv) ?
+			irq_to_desc(dev_priv->lpe_audio.irq) : NULL;
+
+	/**
+	 * mask LPE audio irq before destroying
+	 */
+	if (desc)
+		lpe_audio_irq_mask(&desc->irq_data);
+
+	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_ */