[Intel-gfx] [RFC PATCH v2 1/8] drm/i915: setup bridge for HDMI LPE audio driver

[Pierre-Louis Bossart]

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