Hi,
On 11/8/23 15:10, Andreas Helbech Kleist wrote:
> Hi Hans,
>
> On Wed, 2023-11-08 at 12:25 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 10/24/23 13:29, bingbu.cao@xxxxxxxxx wrote:
>>> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>>
>>> Intel Image Processing Unit 6th Gen includes input and processing
>>> systems
>>> but the hardware presents itself as a single PCI device in system.
>>>
>>> IPU6 PCI device driver basically does PCI configurations and load
>>> the firmware binary, initialises IPU virtual bus, and sets up
>>> platform
>>> specific variants to support multiple IPU6 devices in single device
>>> driver.
>>>
>>> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>> Reported-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>> Suggested-by: Andreas Helbech Kleist <andreaskleist@xxxxxxxxx>
>>> ---
>>> .../media/pci/intel/ipu6/ipu6-platform-regs.h | 179 ++++
>>> drivers/media/pci/intel/ipu6/ipu6.c | 952
>>> ++++++++++++++++++
>>> drivers/media/pci/intel/ipu6/ipu6.h | 352 +++++++
>>> 3 files changed, 1483 insertions(+)
>>> create mode 100644 drivers/media/pci/intel/ipu6/ipu6-platform-
>>> regs.h
>>> create mode 100644 drivers/media/pci/intel/ipu6/ipu6.c
>>> create mode 100644 drivers/media/pci/intel/ipu6/ipu6.h
>>
>> <snip>
>>
>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6.c
>>> b/drivers/media/pci/intel/ipu6/ipu6.c
>>> new file mode 100644
>>> index 000000000000..84960a283daf
>>> --- /dev/null
>>> +++ b/drivers/media/pci/intel/ipu6/ipu6.c
>>> @@ -0,0 +1,952 @@
>>
>> <snip>
>>
>>> +static int ipu6_pci_config_setup(struct pci_dev *dev, u8 hw_ver)
>>> +{
>>> + int ret;
>>> +
>>> + /* disable IPU6 PCI ATS on mtl ES2 */
>>> + if (is_ipu6ep_mtl(hw_ver) && boot_cpu_data.x86_stepping ==
>>> 0x2 &&
>>> + pci_ats_supported(dev))
>>> + pci_disable_ats(dev);
>>> +
>>> + /* No PCI msi capability for IPU6EP */
>>> + if (is_ipu6ep(hw_ver) || is_ipu6ep_mtl(hw_ver)) {
>>> + /* likely do nothing as msi not enabled by default
>>> */
>>> + pci_disable_msi(dev);
>>> + return 0;
>>> + }
>>> +
>>> + ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_MSI);
>>
>> This does not work on TGL systems (and is not reached on ADL and
>> RPL).
>>
>> The out of tree driver instead uses:
>>
>> ret = pci_enable_msi(dev);
>>
>> and that does work correctly on TGL.
>
> Did you see my previous (25//10) comment on the same lines?
>
> pci_alloc_irq_vectors returns number of irqs, so the "if (ret)" check
> following the quoted line is wrong.
Ah right, so a proper fix for this would look something like this:
>From 34de6611d3482d6695dd73eddf7bf3dc1f96883c Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Tue, 7 Nov 2023 11:40:29 +0100
Subject: [PATCH] media: ipu6: Fix interrupts not working on TGL
pci_alloc_irq_vectors() returns the number of successfully allocated
interrupts. Fix the error checking to handle this correctly.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/media/pci/intel/ipu6/ipu6.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/pci/intel/ipu6/ipu6.c b/drivers/media/pci/intel/ipu6/ipu6.c
index b652662fa72e..66565cfdb703 100644
--- a/drivers/media/pci/intel/ipu6/ipu6.c
+++ b/drivers/media/pci/intel/ipu6/ipu6.c
@@ -532,10 +532,12 @@ static int ipu6_pci_config_setup(struct pci_dev *dev, u8 hw_ver)
}
ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_MSI);
- if (ret)
+ if (ret < 0) {
dev_err_probe(&dev->dev, ret, "Request msi failed");
+ return ret;
+ }
- return ret;
+ return 0;
}
static void ipu6_configure_vc_mechanism(struct ipu6_device *isp)
--
2.41.0
Bingbu, please squash something like the above into the next
version of this series.
Regards,
Hans
