From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>
To: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: intel-xe@lists.freedesktop.org
Subject: Re: [Intel-xe] [PATCH 14/21] drm/xe/uapi: Simplify OA configs in uapi
Date: Wed, 04 Oct 2023 08:44:57 -0700 [thread overview]
Message-ID: <87v8bm1jeu.wl-ashutosh.dixit@intel.com> (raw)
In-Reply-To: <ZRzNSRPlZ7mJRCTB@unerlige-ril>
On Tue, 03 Oct 2023 19:26:17 -0700, Umesh Nerlige Ramappa wrote:
>
Hi Umesh,
> On Tue, Sep 19, 2023 at 09:10:42AM -0700, Ashutosh Dixit wrote:
> > In OA uapi, there is no reason to have separate mux/boolean/flex registers
> > in 'struct drm_xe_oa_config'. The kernel knows ranges of these registers
> > and can determine which are which when needed without these being provided
> > through the uapi. Therefore combine the three register arrays into a single
> > one in the uapi.
> >
> > Suggested-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> lgtm
> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Thanks, though just wondering if internally in the driver we should still
maintain the distinction between flex/mux/b_counter registers? Or can we
remove this distinction even internally in the driver and just maintain a
single valid register range per platform? At least at present there seems
to be no reason to maintain 3 different register ranges. So it will
simplify the code a little bit if we just have a single range per
platform (3 ranges can just be maintained in comments). Thoughts?
Thanks.
--
Ashutosh
> > ---
> > drivers/gpu/drm/xe/xe_oa.c | 60 +++++++++-----------------------
> > drivers/gpu/drm/xe/xe_oa_types.h | 8 ++---
> > include/uapi/drm/xe_drm.h | 48 +++++--------------------
> > 3 files changed, 27 insertions(+), 89 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index 63db0969a86b2..19ad23b90e6ad 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -88,9 +88,7 @@ static void xe_oa_config_release(struct kref *ref)
> > struct xe_oa_config *oa_config =
> > container_of(ref, typeof(*oa_config), ref);
> >
> > - kfree(oa_config->flex_regs);
> > - kfree(oa_config->b_counter_regs);
> > - kfree(oa_config->mux_regs);
> > + kfree(oa_config->regs);
> >
> > kfree_rcu(oa_config, rcu);
> > }
> > @@ -970,16 +968,14 @@ static struct xe_oa_config_bo *
> > __xe_oa_alloc_config_buffer(struct xe_oa_stream *stream, struct xe_oa_config *oa_config)
> > {
> > struct xe_oa_config_bo *oa_bo;
> > - size_t config_length = 0;
> > + size_t config_length;
> > struct xe_bb *bb;
> >
> > oa_bo = kzalloc(sizeof(*oa_bo), GFP_KERNEL);
> > if (!oa_bo)
> > return ERR_PTR(-ENOMEM);
> >
> > - config_length += num_lri_dwords(oa_config->mux_regs_len);
> > - config_length += num_lri_dwords(oa_config->b_counter_regs_len);
> > - config_length += num_lri_dwords(oa_config->flex_regs_len);
> > + config_length = num_lri_dwords(oa_config->regs_len);
> > config_length++; /* MI_BATCH_BUFFER_END */
> > config_length = ALIGN(sizeof(u32) * config_length, XE_PAGE_SIZE) / sizeof(u32);
> >
> > @@ -987,9 +983,7 @@ __xe_oa_alloc_config_buffer(struct xe_oa_stream *stream, struct xe_oa_config *oa
> > if (IS_ERR(bb))
> > goto err_free;
> >
> > - write_cs_mi_lri(bb, oa_config->mux_regs, oa_config->mux_regs_len);
> > - write_cs_mi_lri(bb, oa_config->b_counter_regs, oa_config->b_counter_regs_len);
> > - write_cs_mi_lri(bb, oa_config->flex_regs, oa_config->flex_regs_len);
> > + write_cs_mi_lri(bb, oa_config->regs, oa_config->regs_len);
> >
> > oa_bo->bb = bb;
> > oa_bo->oa_config = xe_oa_config_get(oa_config);
> > @@ -1825,6 +1819,13 @@ static bool xe_oa_is_valid_mux_addr(struct xe_oa *oa, u32 addr)
> > return xe_oa_reg_in_range_table(addr, gen12_oa_mux_regs);
> > }
> >
> > +static bool xe_oa_is_valid_config_reg_addr(struct xe_oa *oa, u32 addr)
> > +{
> > + return xe_oa_is_valid_flex_addr(oa, addr) ||
> > + xe_oa_is_valid_b_counter_addr(oa, addr) ||
> > + xe_oa_is_valid_mux_addr(oa, addr);
> > +}
> > +
> > static u32 mask_reg_value(u32 reg, u32 val)
> > {
> > /*
> > @@ -1852,9 +1853,6 @@ xe_oa_alloc_regs(struct xe_oa *oa, bool (*is_valid)(struct xe_oa *oa, u32 addr),
> > int err;
> > u32 i;
> >
> > - if (!n_regs || WARN_ON(!is_valid))
> > - return NULL;
> > -
> > oa_regs = kmalloc_array(n_regs, sizeof(*oa_regs), GFP_KERNEL);
> > if (!oa_regs)
> > return ERR_PTR(-ENOMEM);
> > @@ -1941,9 +1939,7 @@ int xe_oa_add_config_ioctl(struct drm_device *dev, void *data,
> > if (XE_IOCTL_DBG(oa->xe, err))
> > return -EFAULT;
> >
> > - if ((!arg->mux_regs_ptr || !arg->n_mux_regs) &&
> > - (!arg->boolean_regs_ptr || !arg->n_boolean_regs) &&
> > - (!arg->flex_regs_ptr || !arg->n_flex_regs)) {
> > + if (!arg->regs_ptr || !arg->n_regs) {
> > drm_dbg(&oa->xe->drm, "No OA registers given\n");
> > return -EINVAL;
> > }
> > @@ -1964,38 +1960,16 @@ int xe_oa_add_config_ioctl(struct drm_device *dev, void *data,
> > /* Last character in oa_config->uuid will be 0 because oa_config is kzalloc */
> > memcpy(oa_config->uuid, arg->uuid, sizeof(arg->uuid));
> >
> > - oa_config->mux_regs_len = arg->n_mux_regs;
> > - regs = xe_oa_alloc_regs(oa, xe_oa_is_valid_mux_addr,
> > - u64_to_user_ptr(arg->mux_regs_ptr),
> > - arg->n_mux_regs);
> > + oa_config->regs_len = arg->n_regs;
> > + regs = xe_oa_alloc_regs(oa, xe_oa_is_valid_config_reg_addr,
> > + u64_to_user_ptr(arg->regs_ptr),
> > + arg->n_regs);
> > if (IS_ERR(regs)) {
> > drm_dbg(&oa->xe->drm, "Failed to create OA config for mux_regs\n");
> > err = PTR_ERR(regs);
> > goto reg_err;
> > }
> > - oa_config->mux_regs = regs;
> > -
> > - oa_config->b_counter_regs_len = arg->n_boolean_regs;
> > - regs = xe_oa_alloc_regs(oa, xe_oa_is_valid_b_counter_addr,
> > - u64_to_user_ptr(arg->boolean_regs_ptr),
> > - arg->n_boolean_regs);
> > - if (IS_ERR(regs)) {
> > - drm_dbg(&oa->xe->drm, "Failed to create OA config for b_counter_regs\n");
> > - err = PTR_ERR(regs);
> > - goto reg_err;
> > - }
> > - oa_config->b_counter_regs = regs;
> > -
> > - oa_config->flex_regs_len = arg->n_flex_regs;
> > - regs = xe_oa_alloc_regs(oa, xe_oa_is_valid_flex_addr,
> > - u64_to_user_ptr(arg->flex_regs_ptr),
> > - arg->n_flex_regs);
> > - if (IS_ERR(regs)) {
> > - drm_dbg(&oa->xe->drm, "Failed to create OA config for flex_regs\n");
> > - err = PTR_ERR(regs);
> > - goto reg_err;
> > - }
> > - oa_config->flex_regs = regs;
> > + oa_config->regs = regs;
> >
> > err = mutex_lock_interruptible(&oa->metrics_lock);
> > if (err)
> > diff --git a/drivers/gpu/drm/xe/xe_oa_types.h b/drivers/gpu/drm/xe/xe_oa_types.h
> > index 126692718c888..ac8b23695cc6e 100644
> > --- a/drivers/gpu/drm/xe/xe_oa_types.h
> > +++ b/drivers/gpu/drm/xe/xe_oa_types.h
> > @@ -52,12 +52,8 @@ struct xe_oa_config {
> > char uuid[UUID_STRING_LEN + 1];
> > int id;
> >
> > - const struct xe_oa_reg *mux_regs;
> > - u32 mux_regs_len;
> > - const struct xe_oa_reg *b_counter_regs;
> > - u32 b_counter_regs_len;
> > - const struct xe_oa_reg *flex_regs;
> > - u32 flex_regs_len;
> > + const struct xe_oa_reg *regs;
> > + u32 regs_len;
> >
> > struct attribute_group sysfs_metric;
> > struct attribute *attrs[2];
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index bf0af9474e7ee..fe873dc63fc5a 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -1292,52 +1292,20 @@ struct drm_xe_oa_config {
> > char uuid[36];
> >
> > /**
> > - * @n_mux_regs:
> > + * @n_regs:
> > *
> > - * Number of mux regs in &mux_regs_ptr.
> > + * Number of regs in @regs_ptr.
> > */
> > - __u32 n_mux_regs;
> > + __u32 n_regs;
> >
> > /**
> > - * @n_boolean_regs:
> > + * @regs_ptr:
> > *
> > - * Number of boolean regs in &boolean_regs_ptr.
> > + * Pointer to tuples of u32 values (register address, value) for OA
> > + * config registers. Expected length of buffer is (2 * sizeof(u32) *
> > + * @n_regs).
> > */
> > - __u32 n_boolean_regs;
> > -
> > - /**
> > - * @n_flex_regs:
> > - *
> > - * Number of flex regs in &flex_regs_ptr.
> > - */
> > - __u32 n_flex_regs;
> > -
> > - /**
> > - * @mux_regs_ptr:
> > - *
> > - * Pointer to tuples of u32 values (register address, value) for mux
> > - * registers. Expected length of buffer is (2 * sizeof(u32) *
> > - * &n_mux_regs).
> > - */
> > - __u64 mux_regs_ptr;
> > -
> > - /**
> > - * @boolean_regs_ptr:
> > - *
> > - * Pointer to tuples of u32 values (register address, value) for mux
> > - * registers. Expected length of buffer is (2 * sizeof(u32) *
> > - * &n_boolean_regs).
> > - */
> > - __u64 boolean_regs_ptr;
> > -
> > - /**
> > - * @flex_regs_ptr:
> > - *
> > - * Pointer to tuples of u32 values (register address, value) for mux
> > - * registers. Expected length of buffer is (2 * sizeof(u32) *
> > - * &n_flex_regs).
> > - */
> > - __u64 flex_regs_ptr;
> > + __u64 regs_ptr;
> > };
> >
> > /*
> > --
> > 2.41.0
> >
next prev parent reply other threads:[~2023-10-04 15:45 UTC|newest]
Thread overview: 88+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-19 16:10 [Intel-xe] [PATCH 00/21] Add OA functionality to Xe Ashutosh Dixit
2023-09-19 16:10 ` [Intel-xe] [PATCH 01/21] drm/xe/uapi: Introduce OA (observability architecture) uapi Ashutosh Dixit
2023-10-04 0:26 ` Umesh Nerlige Ramappa
2023-10-04 0:36 ` Dixit, Ashutosh
2023-11-04 1:23 ` Dixit, Ashutosh
2023-09-19 16:10 ` [Intel-xe] [PATCH 02/21] drm/xe/oa: Add OA types Ashutosh Dixit
2023-10-13 17:05 ` Umesh Nerlige Ramappa
2023-09-19 16:10 ` [Intel-xe] [PATCH 03/21] drm/xe/oa: Add registers and GPU commands used by OA Ashutosh Dixit
2023-10-13 17:06 ` Umesh Nerlige Ramappa
2023-11-17 22:52 ` Dixit, Ashutosh
2023-09-19 16:10 ` [Intel-xe] [PATCH 04/21] drm/xe/oa: Module init/exit and probe/remove Ashutosh Dixit
2023-10-13 17:50 ` Umesh Nerlige Ramappa
2023-10-20 7:08 ` [Intel-xe] [04/21] " Lionel Landwerlin
2023-10-27 20:28 ` Dixit, Ashutosh
2023-09-19 16:10 ` [Intel-xe] [PATCH 05/21] drm/xe/oa: Add/remove config ioctl's Ashutosh Dixit
2023-10-13 17:59 ` Umesh Nerlige Ramappa
2023-09-19 16:10 ` [Intel-xe] [PATCH 06/21] drm/xe/oa: Start implementing OA stream open ioctl Ashutosh Dixit
2023-10-13 18:09 ` Umesh Nerlige Ramappa
2023-09-19 16:10 ` [Intel-xe] [PATCH 07/21] drm/xe/oa: OA stream initialization Ashutosh Dixit
2023-10-04 15:22 ` Dixit, Ashutosh
2023-09-19 16:10 ` [Intel-xe] [PATCH 08/21] drm/xe/oa: Expose OA stream fd Ashutosh Dixit
2023-10-13 18:17 ` Umesh Nerlige Ramappa
2023-09-19 16:10 ` [Intel-xe] [PATCH 09/21] drm/xe/oa: Read file_operation Ashutosh Dixit
2023-10-14 0:56 ` Umesh Nerlige Ramappa
2023-09-19 16:10 ` [Intel-xe] [PATCH 10/21] drm/xe/oa: Implement queries Ashutosh Dixit
2023-10-14 0:58 ` Umesh Nerlige Ramappa
2023-09-19 16:10 ` [Intel-xe] [PATCH 11/21] drm/xe/oa: Override GuC RC with OA on PVC Ashutosh Dixit
2023-10-16 17:43 ` Umesh Nerlige Ramappa
2023-09-19 16:10 ` [Intel-xe] [PATCH 12/21] drm/xe/uapi: "Perf" layer to support multiple perf counter stream types Ashutosh Dixit
2023-10-04 2:13 ` Umesh Nerlige Ramappa
2023-10-05 4:33 ` Dixit, Ashutosh
2023-09-19 16:10 ` [Intel-xe] [PATCH 13/21] drm/xe/uapi: Multiplex PERF ops through a single PERF ioctl Ashutosh Dixit
2023-10-04 2:23 ` Umesh Nerlige Ramappa
2023-10-05 5:27 ` Dixit, Ashutosh
2023-10-05 15:22 ` Dixit, Ashutosh
2023-10-05 18:27 ` Umesh Nerlige Ramappa
2023-10-05 23:18 ` Dixit, Ashutosh
2023-09-19 16:10 ` [Intel-xe] [PATCH 14/21] drm/xe/uapi: Simplify OA configs in uapi Ashutosh Dixit
2023-10-04 2:26 ` Umesh Nerlige Ramappa
2023-10-04 15:44 ` Dixit, Ashutosh [this message]
2023-10-04 16:13 ` Rodrigo Vivi
2023-09-19 16:10 ` [Intel-xe] [PATCH 15/21] drm/xe/uapi: Remove OA format names from OA uapi Ashutosh Dixit
2023-10-04 2:33 ` Umesh Nerlige Ramappa
2023-10-05 6:13 ` Dixit, Ashutosh
2023-09-19 16:10 ` [Intel-xe] [PATCH 16/21] drm/xe/oa: Make xe_oa_timestamp_frequency per gt Ashutosh Dixit
2023-09-21 20:45 ` Rodrigo Vivi
2023-09-21 21:58 ` Dixit, Ashutosh
2023-09-22 19:10 ` Rodrigo Vivi
2023-09-19 16:10 ` [Intel-xe] [PATCH 17/21] drm/xe/oa: Remove filtering reports on context id Ashutosh Dixit
2023-10-14 1:01 ` Umesh Nerlige Ramappa
2023-10-20 7:30 ` [Intel-xe] [17/21] " Lionel Landwerlin
2023-10-20 17:00 ` Umesh Nerlige Ramappa
2023-09-19 16:10 ` [Intel-xe] [PATCH 18/21] drm/xe/uapi: More OA uapi fixes/additions Ashutosh Dixit
2023-10-04 0:23 ` Dixit, Ashutosh
2023-10-05 22:33 ` Dixit, Ashutosh
2023-10-12 3:14 ` Umesh Nerlige Ramappa
2023-10-20 7:28 ` [Intel-xe] [18/21] " Lionel Landwerlin
2023-10-27 20:28 ` Dixit, Ashutosh
2023-10-30 10:06 ` Lionel Landwerlin
2023-10-31 2:08 ` Dixit, Ashutosh
2023-09-19 16:10 ` [Intel-xe] [PATCH 19/21] drm/xe/uapi: Drop OA_IOCTL_VERSION Ashutosh Dixit
2023-09-19 17:02 ` Dixit, Ashutosh
2023-10-04 2:37 ` Umesh Nerlige Ramappa
2023-10-05 3:28 ` Dixit, Ashutosh
2023-10-05 19:35 ` Umesh Nerlige Ramappa
2023-10-20 7:36 ` [Intel-xe] [19/21] " Lionel Landwerlin
2023-10-23 23:02 ` Umesh Nerlige Ramappa
2023-10-24 4:08 ` Dixit, Ashutosh
2023-10-24 15:54 ` Dixit, Ashutosh
2023-09-19 16:10 ` [Intel-xe] [PATCH 20/21] drm/xe/uapi: Use OA unit id to identify OA unit Ashutosh Dixit
2023-10-04 22:37 ` Umesh Nerlige Ramappa
2023-10-05 3:04 ` Dixit, Ashutosh
2023-10-05 3:09 ` Dixit, Ashutosh
2023-09-19 16:10 ` [Intel-xe] [PATCH 21/21] drm/xe/uapi: Convert OA property key/value pairs to a struct Ashutosh Dixit
2023-09-21 23:53 ` Dixit, Ashutosh
2023-10-05 5:37 ` Dixit, Ashutosh
2023-10-05 19:26 ` Umesh Nerlige Ramappa
2023-09-19 16:19 ` [Intel-xe] ✓ CI.Patch_applied: success for Add OA functionality to Xe (rev6) Patchwork
2023-09-19 16:19 ` [Intel-xe] ✗ CI.checkpatch: warning " Patchwork
2023-09-19 16:21 ` [Intel-xe] ✓ CI.KUnit: success " Patchwork
2023-09-19 16:28 ` [Intel-xe] ✓ CI.Build: " Patchwork
2023-09-19 16:28 ` [Intel-xe] ✗ CI.Hooks: failure " Patchwork
2023-09-19 16:29 ` [Intel-xe] ✓ CI.checksparse: success " Patchwork
2023-09-19 17:04 ` [Intel-xe] ✗ CI.BAT: failure " Patchwork
2023-10-14 1:05 ` [Intel-xe] [PATCH 00/21] Add OA functionality to Xe Umesh Nerlige Ramappa
2023-10-20 7:44 ` Lionel Landwerlin
2023-10-20 7:52 ` Lionel Landwerlin
2023-10-31 6:51 ` Dixit, Ashutosh
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=87v8bm1jeu.wl-ashutosh.dixit@intel.com \
--to=ashutosh.dixit@intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=umesh.nerlige.ramappa@intel.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.