Andreas,
------------------------------------------------------------------------
BRs,
Bingbu Cao
>-----Original Message-----
>From: Andreas Helbech Kleist <andreaskleist@xxxxxxxxx>
>Sent: Wednesday, November 8, 2023 10:11 PM
>To: Hans de Goede <hans@xxxxxxxxx>; Cao, Bingbu <bingbu.cao@xxxxxxxxx>;
>linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx;
>laurent.pinchart@xxxxxxxxxxxxxxxx
>Cc: andriy.shevchenko@xxxxxxxxxxxxxxx; hdegoede@xxxxxxxxxx;
>ilpo.jarvinen@xxxxxxxxxxxxxxx; claus.stovgaard@xxxxxxxxx;
>tfiga@xxxxxxxxxxxx; senozhatsky@xxxxxxxxxxxx;
>tomi.valkeinen@xxxxxxxxxxxxxxxx; bingbu.cao@xxxxxxxxxxxxxxx; Qiu, Tian Shu
><tian.shu.qiu@xxxxxxxxx>; Wang, Hongju <hongju.wang@xxxxxxxxx>
>Subject: Re: [PATCH v2 01/15] media: intel/ipu6: add Intel IPU6 PCI device
>driver
>
>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.
Andreas,
You are right, the actual problem is the return value checking is wrong.
Hans, thanks for your report. I don't have TGL device now. ☹
I will fix it in next version.
>
>/Andreas
