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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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
> 

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