Re: [igt-dev] [PATCH i-g-t v4 3/6] lib/intel_mmio: use igt_device_map_pci_bar_region for mmap if fd is known.

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniel Mrzyglod <daniel.t.mrzyglod@intel.com>,
	igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t v4 3/6] lib/intel_mmio: use igt_device_map_pci_bar_region for mmap if fd is known.
Date: Fri, 26 Apr 2019 06:40:31 +0100	[thread overview]
Message-ID: <155625723098.3869.5434947766903910774@skylake-alporthouse-com> (raw)
In-Reply-To: <20190425205822.8995-4-daniel.t.mrzyglod@intel.com>

Quoting Daniel Mrzyglod (2019-04-25 21:58:19)
>  void
> -intel_mmio_use_pci_bar(struct pci_device *pci_dev)
> +intel_mmio_use_pci_bar(struct pci_device *pci_dev, int fd)
>  {
>         uint32_t devid, gen;
>         int mmio_bar, mmio_size;
> @@ -135,14 +136,28 @@ intel_mmio_use_pci_bar(struct pci_device *pci_dev)
>         else
>                 mmio_size = 2*1024*1024;
>  
> -       error = pci_device_map_range (pci_dev,
> -                                     pci_dev->regions[mmio_bar].base_addr,
> -                                     mmio_size,
> -                                     PCI_DEV_MAP_FLAG_WRITABLE,
> -                                     &igt_global_mmio);
> +       /* It's for some legacy tools existing in tree that require
> +        * working without driver
> +        */
> +
> +       if (fd == -1) {
> +               error = pci_device_map_range(pci_dev,
> +                                            pci_dev->regions[mmio_bar].base_addr,
> +                                            mmio_size,
> +                                            PCI_DEV_MAP_FLAG_WRITABLE,
> +                                            &igt_global_mmio);
> +
> +               igt_fail_on_f(error != 0, "Couldn't map MMIO region\n");
> +       } else {
> +               /* This method is much more convenient when we have many
> +                * concurrent PCI devices
> +                */
> +               igt_global_mmio = igt_device_map_pci_bar_region(fd, mmio_bar,
> +                                                               mmio_size);
> +               igt_fail_on_f(mmio_data->igt_mmio == MAP_FAILED,
> +                             "Couldn't map MMIO region\n");

This makes no sense to me at all. We already have the pci_device we
want and the BAR we want to mmap, so why repeat ourselves?

If you didn't want that pci_dev, why are we passing around the wrong
one?
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev