[igt-dev] [PATCH i-g-t v3 2/5] lib/igt_device: add igt_device_map_pci_bar_region

[Katarzyna Dec]

On Mon, Apr 15, 2019 at 10:59:34AM +0200, Daniel Mrzyglod wrote:
> This function use sysfs to map particular mmio region.
s/use/uses/
> fd of opened device point us for specific pci device.
s/point/points
(not a merge blocker)
> 
> Signed-off-by: Daniel Mrzyglod <daniel.t.mrzyglod at intel.com>
> ---
>  lib/igt_device.c | 38 ++++++++++++++++++++++++++++++++++++++
>  lib/igt_device.h |  1 +
>  2 files changed, 39 insertions(+)
> 
> diff --git a/lib/igt_device.c b/lib/igt_device.c
> index f3d2a9f5..8b280474 100644
> --- a/lib/igt_device.c
> +++ b/lib/igt_device.c
> @@ -225,3 +225,41 @@ struct pci_device *igt_device_get_pci_device(int fd)
>  
>  	return pci_dev;
>  }
> +
> +/**
> + * igt_device_map_pci_bar_region:
> + *
> + * @fd: the device
> + * @mmio_bar: bar to be mapped
> + * @mmio_size: bar size
> + *
> + * Returns:
> + * The pointer for mmapped bar
> + */
> +void *igt_device_map_pci_bar_region(int fd, int  mmio_bar, int mmio_size)
> +{
> +	struct igt_pci_addr pci_addr;
> +	char *filepath = NULL;
> +	int newfd;
> +	void *igt_mmio;
> +
> +	if (igt_device_get_pci_addr(fd, &pci_addr)) {
> +		igt_warn("Unable to find device PCI address\n");
> +		return NULL;
> +	}
> +
> +	asprintf(&filepath, "/sys/devices/pci%.4x:%.2x/%.4x:%.2x:%.2x.%.1x/resource%.1x",
> +		 pci_addr.domain, pci_addr.bus, pci_addr.domain, pci_addr.bus,
> +		 pci_addr.device, pci_addr.function, mmio_bar);
First this looks like a nightmatre for the person that is debugging. 
Maybe is will be easier to use readlinkat for getting device path and
igt_sysfs_set for writing to resource?
> +	newfd = open(filepath, O_RDWR | O_SYNC);
> +	igt_mmio = mmap(0, mmio_size, PROT_READ | PROT_WRITE, MAP_SHARED,
> +			newfd, 0);
> +	free(filepath);
> +	close(newfd);
Have you applied comment from Chris from last round? (I cannot see any changes,
but maybe you discussed with him on IRC about that).

> +	igt_fail_on_f(igt_mmio == MAP_FAILED,
> +		      "Couldn't map MMIO region %.4x:%.2x:%.2x.%.1x BAR %d\n",
If you will be using this '%.4x:%.2x:%.2x.%.1x' in more than 1 place in this
function, I suggest to make a variable of this - this will decrease possiblity
of errors and will be easier to read.

Kasia
> +		      pci_addr.domain, pci_addr.bus, pci_addr.device,
> +		      pci_addr.function, mmio_bar);
> +
> +	return igt_mmio;
> +}
> diff --git a/lib/igt_device.h b/lib/igt_device.h
> index 860b3a8a..6ffc1d5e 100644
> --- a/lib/igt_device.h
> +++ b/lib/igt_device.h
> @@ -34,5 +34,6 @@ void igt_device_drop_master(int fd);
>  
>  int igt_device_get_card_index(int fd);
>  struct pci_device *igt_device_get_pci_device(int fd);
> +void *igt_device_map_pci_bar_region(int fd, int  mmio_bar, int mmio_size);
>  
>  #endif /* __IGT_DEVICE_H__ */
> -- 
> 2.20.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev