[igt-dev] [PATCH i-g-t v7 3/3] lib/intel_mmio: add additional api for multiple devices

[Chris Wilson]

Quoting Daniel Mrzyglod (2019-08-16 15:53:17)
> Library  was limited for reading registers for only
> one device at a time in igt tests.
> Changes in this patch give as oportunity to test multiple devices in
> the same time.
> 
> v7: remove unnecessary code
> v6: Reword patch. Cosmetic changes.
> 
> Cc: Antonio Argenziano <antonio.argenziano at intel.com>
> Cc: Daniele Spurio Ceraolo <daniele.ceraolospurio at intel.com>
> Cc: Katarzyna Dec <katarzyna.dec at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> 
> Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> Signed-off-by: Daniel Mrzyglod <daniel.t.mrzyglod at intel.com>
> ---
>  benchmarks/gem_latency.c      |   3 +-
>  benchmarks/gem_wsim.c         |   3 +-
>  lib/intel_io.h                |  82 ++++++++++++++++---------
>  lib/intel_iosf.c              |  74 +++++++++++++----------
>  lib/intel_mmio.c              | 111 ++++++++++++++++++----------------
>  tests/i915/gem_exec_latency.c |   4 +-
>  tests/i915/gem_exec_parse.c   |  13 ++--
>  tests/i915/gem_workarounds.c  |   3 +-
>  tests/i915/i915_pm_lpsp.c     |   6 +-
>  tools/intel_audio_dump.c      |   6 +-
>  tools/intel_backlight.c       |   3 +-
>  tools/intel_display_poller.c  |   5 +-
>  tools/intel_forcewaked.c      |  15 ++---
>  tools/intel_gpu_time.c        |   3 +-
>  tools/intel_infoframes.c      |   5 +-
>  tools/intel_l3_parity.c       |  11 ++--
>  tools/intel_lid.c             |   3 +-
>  tools/intel_panel_fitter.c    |   5 +-
>  tools/intel_perf_counters.c   |  10 +--
>  tools/intel_reg.c             |  23 +++----
>  tools/intel_reg_checker.c     |   3 +-
>  tools/intel_watermark.c       |  42 +++++++------
>  22 files changed, 250 insertions(+), 183 deletions(-)
> 
> diff --git a/benchmarks/gem_latency.c b/benchmarks/gem_latency.c
> index c3fc4bf0..569faa58 100644
> --- a/benchmarks/gem_latency.c
> +++ b/benchmarks/gem_latency.c
> @@ -55,6 +55,7 @@
>  static int done;
>  static int fd;
>  static volatile uint32_t *timestamp_reg;
> +static struct mmio_data mmio_data;

struct mmio_data is a library defined type. Please give the type an
appropriate name.

> +struct intel_register_range {
> +       uint32_t base;
> +       uint32_t size;
> +       uint32_t flags;
> +};
> +
> +struct intel_register_map {
> +       struct intel_register_range *map;
> +       uint32_t top;
> +       uint32_t alignment_mask;
> +};
> +
> +struct mmio_data {
> +       int inited;

Does anyone really expect to call init() twice and have it work?

> +       bool safe;
> +       uint32_t i915_devid;

You were passed a pci_device for initialisation. Nothing i915 about it.

> +       struct intel_register_map map;
> +       int key;
> +       void *igt_mmio;

Please consider the holes and repack.
-Chris