From: Katarzyna Dec <katarzyna.dec@intel.com>
To: Daniel Mrzyglod <daniel.t.mrzyglod@intel.com>
Cc: igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t 3/3] lib/intel_mmio: Remove global igt_global_mmio
Date: Thu, 4 Apr 2019 16:47:46 +0200 [thread overview]
Message-ID: <20190404144746.GF12599@kdec5-desk.ger.corp.intel.com> (raw)
In-Reply-To: <20190404131805.18110-4-daniel.t.mrzyglod@intel.com>
On Thu, Apr 04, 2019 at 03:18:05PM +0200, Daniel Mrzyglod wrote:
> In future tests there will be multiple PCI devices run at once.
> It's good for them to have different mmio space.
>
> Signed-off-by: Daniel Mrzyglod <daniel.t.mrzyglod@intel.com>
> ---
> benchmarks/gem_latency.c | 11 +-
> benchmarks/gem_wsim.c | 8 +-
> lib/intel_io.h | 93 +++++++-----
> lib/intel_iosf.c | 82 +++++++----
> lib/intel_mmio.c | 206 +++++++++++++++-----------
> tests/i915/gem_exec_latency.c | 10 +-
> tests/i915/gem_exec_parse.c | 14 +-
> tests/i915/i915_pm_lpsp.c | 8 +-
> tools/intel_audio_dump.c | 267 +++++++++++++++++-----------------
> tools/intel_backlight.c | 13 +-
> tools/intel_display_poller.c | 13 +-
> tools/intel_forcewaked.c | 11 +-
> tools/intel_gpu_time.c | 7 +-
> tools/intel_infoframes.c | 74 +++++-----
> tools/intel_l3_parity.c | 12 +-
> tools/intel_lid.c | 5 +-
> tools/intel_panel_fitter.c | 29 ++--
> tools/intel_perf_counters.c | 11 +-
> tools/intel_reg.c | 33 +++--
> tools/intel_reg_checker.c | 6 +-
> tools/intel_watermark.c | 37 ++---
> 21 files changed, 527 insertions(+), 423 deletions(-)
>
> diff --git a/benchmarks/gem_latency.c b/benchmarks/gem_latency.c
> index c3fc4bf0..ea58098d 100644
> --- a/benchmarks/gem_latency.c
> +++ b/benchmarks/gem_latency.c
> @@ -43,6 +43,7 @@
> #include <sys/poll.h>
> #include <sys/resource.h>
> #include "drm.h"
> +#include "igt_device.h"
>
> #define LOCAL_I915_EXEC_FENCE_IN (1<<16)
> #define LOCAL_I915_EXEC_FENCE_OUT (1<<17)
> @@ -55,9 +56,12 @@
> static int done;
> static int fd;
> static volatile uint32_t *timestamp_reg;
> +struct mmio_data igt_global_mmio;
>
> -#define REG(x) (volatile uint32_t *)((volatile char *)igt_global_mmio + x)
> -#define REG_OFFSET(x) ((volatile char *)(x) - (volatile char *)igt_global_mmio)
> +#define REG(x) \
> + (volatile uint32_t *)((volatile char *)igt_global_mmio.igt_mmio + x)
> +#define REG_OFFSET(x) \
> + ((volatile char *)(x) - (volatile char *)igt_global_mmio.igt_mmio)
>
> #if defined(__USE_XOPEN2K) && defined(gen7_safe_mmio)
> static pthread_spinlock_t timestamp_lock;
> @@ -456,7 +460,8 @@ static int run(int seconds,
> if (gen < 6)
> return IGT_EXIT_SKIP; /* Needs BCS timestamp */
>
> - intel_register_access_init(intel_get_pci_device(), false, fd);
> + intel_register_access_init(igt_device_get_pci_device(fd), false, fd,
> + &igt_global_mmio);
>
> if (gen == 6)
> timestamp_reg = REG(RCS_TIMESTAMP);
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index afb9644d..274182a3 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -50,6 +50,7 @@
>
> #include "intel_io.h"
> #include "igt_aux.h"
> +#include "igt_device.h"
> #include "igt_rand.h"
> #include "igt_perf.h"
> #include "sw_sync.h"
> @@ -57,6 +58,7 @@
>
> #include "ewma.h"
>
> +struct mmio_data igt_global_mmio;
> #define LOCAL_I915_EXEC_FENCE_IN (1<<16)
> #define LOCAL_I915_EXEC_FENCE_OUT (1<<17)
>
> @@ -229,7 +231,8 @@ static int fd;
> #define SEQNO_OFFSET(engine) (SEQNO_IDX(engine) * sizeof(uint32_t))
>
> #define RCS_TIMESTAMP (0x2000 + 0x358)
> -#define REG(x) (volatile uint32_t *)((volatile char *)igt_global_mmio + x)
> +#define REG(x) \
> + (volatile uint32_t *)((volatile char *)igt_global_mmio.igt_mmio + x)
>
> static const char *ring_str_map[NUM_ENGINES] = {
> [RCS] = "RCS",
> @@ -2223,7 +2226,8 @@ static void init_clocks(void)
> uint32_t rcs_start, rcs_end;
> double overhead, t;
>
> - intel_register_access_init(intel_get_pci_device(), false, fd);
> + intel_register_access_init(igt_device_get_pci_device(fd), false, fd,
> + &igt_global_mmio);
>
> if (verbose <= 1)
> return;
> diff --git a/lib/intel_io.h b/lib/intel_io.h
> index 6014c485..35fab8e8 100644
> --- a/lib/intel_io.h
> +++ b/lib/intel_io.h
> @@ -30,37 +30,67 @@
>
> #include <stdint.h>
> #include <pciaccess.h>
> +#include <stdbool.h>
>
> /* register access helpers from intel_mmio.c */
> -extern void *igt_global_mmio;
> -void intel_mmio_use_pci_bar(struct pci_device *pci_dev);
> -void intel_mmio_use_dump_file(char *file);
>
> -int intel_register_access_init(struct pci_device *pci_dev, int safe, int fd);
> -void intel_register_access_fini(void);
> -uint32_t intel_register_read(uint32_t reg);
> -void intel_register_write(uint32_t reg, uint32_t val);
> -int intel_register_access_needs_fakewake(void);
> +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;
> + bool safe;
> + uint32_t i915_devid;
> + struct intel_register_map map;
> + int key;
> + void *igt_mmio;
> +};
>
> -uint32_t INREG(uint32_t reg);
> -uint16_t INREG16(uint32_t reg);
> -uint8_t INREG8(uint32_t reg);
> -void OUTREG(uint32_t reg, uint32_t val);
> -void OUTREG16(uint32_t reg, uint16_t val);
> -void OUTREG8(uint32_t reg, uint8_t val);
> +void intel_mmio_use_pci_bar(struct pci_device *pci_dev, int fd,
> + struct mmio_data *mmio_data);
> +void intel_mmio_use_dump_file(struct mmio_data *mmio_data, char *file);
> +
> +int intel_register_access_init(struct pci_device *pci_dev, int safe, int fd,
> + struct mmio_data *mmio_data);
> +void intel_register_access_fini(struct mmio_data *mmio_data);
> +uint32_t intel_register_read(struct mmio_data *mmio_data, uint32_t reg);
> +void intel_register_write(struct mmio_data *mmio_data, uint32_t reg,
> + uint32_t val);
> +int intel_register_access_needs_fakewake(struct mmio_data *mmio_data);
> +
> +uint32_t INREG(struct mmio_data *mmio_data, uint32_t reg);
> +uint16_t INREG16(struct mmio_data *mmio_data, uint32_t reg);
> +uint8_t INREG8(struct mmio_data *mmio_data, uint32_t reg);
> +void OUTREG(struct mmio_data *mmio_data, uint32_t reg, uint32_t val);
> +void OUTREG16(struct mmio_data *mmio_data, uint32_t reg, uint16_t val);
> +void OUTREG8(struct mmio_data *mmio_data, uint32_t reg, uint8_t val);
>
> /* sideband access functions from intel_iosf.c */
> -uint32_t intel_dpio_reg_read(uint32_t reg, int phy);
> -void intel_dpio_reg_write(uint32_t reg, uint32_t val, int phy);
> -uint32_t intel_flisdsi_reg_read(uint32_t reg);
> -void intel_flisdsi_reg_write(uint32_t reg, uint32_t val);
> -uint32_t intel_iosf_sb_read(uint32_t port, uint32_t reg);
> -void intel_iosf_sb_write(uint32_t port, uint32_t reg, uint32_t val);
> +uint32_t intel_dpio_reg_read(struct mmio_data *mmio_data, uint32_t reg,
> + int phy);
> +void intel_dpio_reg_write(struct mmio_data *mmio_data, uint32_t reg,
> + uint32_t val, int phy);
> +uint32_t intel_flisdsi_reg_read(struct mmio_data *mmio_data, uint32_t reg);
> +void intel_flisdsi_reg_write(struct mmio_data *mmio_data, uint32_t reg,
> + uint32_t val);
> +uint32_t intel_iosf_sb_read(struct mmio_data *mmio_data, uint32_t port,
> + uint32_t reg);
> +void intel_iosf_sb_write(struct mmio_data *mmio_data, uint32_t port,
> + uint32_t reg, uint32_t val);
>
> -int intel_punit_read(uint32_t addr, uint32_t *val);
> -int intel_punit_write(uint32_t addr, uint32_t val);
> -int intel_nc_read(uint32_t addr, uint32_t *val);
> -int intel_nc_write(uint32_t addr, uint32_t val);
> +int intel_punit_read(struct mmio_data *mmio_data, uint32_t addr, uint32_t *val);
> +int intel_punit_write(struct mmio_data *mmio_data, uint32_t addr, uint32_t val);
> +int intel_nc_read(struct mmio_data *mmio_data, uint32_t addr, uint32_t *val);
> +int intel_nc_write(struct mmio_data *mmio_data, uint32_t addr, uint32_t val);
>
> /* register maps from intel_reg_map.c */
> #ifndef __GTK_DOC_IGNORE__
> @@ -71,19 +101,10 @@ int intel_nc_write(uint32_t addr, uint32_t val);
> #define INTEL_RANGE_RW (INTEL_RANGE_READ | INTEL_RANGE_WRITE)
> #define INTEL_RANGE_END (1<<31)
>
> -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 intel_register_map intel_get_register_map(uint32_t devid);
> -struct intel_register_range *intel_get_register_range(struct intel_register_map map, uint32_t offset, uint32_t mode);
> +struct intel_register_range *intel_get_register_range(struct intel_register_map
> + map, uint32_t offset,
> + uint32_t mode);
> #endif /* __GTK_DOC_IGNORE__ */
>
> #endif /* INTEL_GPU_TOOLS_H */
> diff --git a/lib/intel_iosf.c b/lib/intel_iosf.c
> index 3b5a1370..52a885f5 100644
> --- a/lib/intel_iosf.c
> +++ b/lib/intel_iosf.c
> @@ -19,8 +19,8 @@
> /* Private register write, double-word addressing, non-posted */
> #define SB_CRWRDA_NP 0x07
>
> -static int vlv_sideband_rw(uint32_t port, uint8_t opcode, uint32_t addr,
> - uint32_t *val)
> +static int vlv_sideband_rw(struct mmio_data *mmio_data, uint32_t port,
> + uint8_t opcode, uint32_t addr, uint32_t *val)
As I undestand VLV is quite an old gen, do we need to add mmio_data here? Or not
adding it breaks some compatibility?
nit: patch is almost 3000 lines of different changes, please divide it in
smaller chunks. It is hard to read so large changes and yet we need to
understand what is going on :/
Kasia
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
next prev parent reply other threads:[~2019-04-04 14:47 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-04 13:18 [igt-dev] [PATCH i-g-t 0/3] Remove global igt_global_mmio Daniel Mrzyglod
2019-04-04 13:18 ` [igt-dev] [PATCH i-g-t 1/3] lib/igt_device: add igt_device_get_pci_addr by fd Daniel Mrzyglod
2019-04-04 14:17 ` Katarzyna Dec
2019-04-08 10:16 ` Petri Latvala
2019-04-08 11:14 ` Jani Nikula
2019-04-04 13:18 ` [igt-dev] [PATCH i-g-t 2/3] lib/igt_device: add igt_map_bar_region Daniel Mrzyglod
2019-04-04 13:30 ` Chris Wilson
2019-04-04 14:23 ` Katarzyna Dec
2019-04-04 13:18 ` [igt-dev] [PATCH i-g-t 3/3] lib/intel_mmio: Remove global igt_global_mmio Daniel Mrzyglod
2019-04-04 14:47 ` Katarzyna Dec [this message]
2019-04-04 15:27 ` Mrzyglod, Daniel T
2019-04-05 13:51 ` Katarzyna Dec
2019-04-04 14:57 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2019-04-05 7:10 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190404144746.GF12599@kdec5-desk.ger.corp.intel.com \
--to=katarzyna.dec@intel.com \
--cc=daniel.t.mrzyglod@intel.com \
--cc=igt-dev@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox