On Fri, 14 Oct 2016, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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
>>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
