From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5C16BE7C4D5 for ; Wed, 4 Oct 2023 15:45:01 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 03A9410E10B; Wed, 4 Oct 2023 15:45:01 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTPS id E9E7910E10B for ; Wed, 4 Oct 2023 15:44:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1696434298; x=1727970298; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=2LkxPSLZLnNmzOaQdhI36JMQhew6TIS/rC5m7g72OWE=; b=jlChsMJjpgAlW5EtZr7r9+PPTDzKcg34k/2RjICWfZnQ8ghk4O05LK+K VewgZkdeon7kyRpyZ+6sv6y8stQQh9u2UUvovjnJ69/DQLJ7uLK6kzxNX SjcB0/Pt+yHxJON7vN5sh0JGYIp/9TVbv95hrtsgep2vi7jOTL6RbUdrO dSrQiUDDSew1nwF4jm1MHSpy6qutCxDAdOlEbuinFW21esNCy07gX1BII YcRnHe6Ml91yLr3Vsr9M5mtcevHJvOm426Pv+RxKfZRh8ifULod9GMASD KUiAixh0MNVLTDIhVBMnsyEN0s6USZoNQFB6xfGHKr0obrT3cFtfGaG0o w==; X-IronPort-AV: E=McAfee;i="6600,9927,10853"; a="382068895" X-IronPort-AV: E=Sophos;i="6.03,200,1694761200"; d="scan'208";a="382068895" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Oct 2023 08:44:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10853"; a="745027585" X-IronPort-AV: E=Sophos;i="6.03,200,1694761200"; d="scan'208";a="745027585" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.101.181]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Oct 2023 08:44:57 -0700 Date: Wed, 04 Oct 2023 08:44:57 -0700 Message-ID: <87v8bm1jeu.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa In-Reply-To: References: <20230919161049.2307855-1-ashutosh.dixit@intel.com> <20230919161049.2307855-15-ashutosh.dixit@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [Intel-xe] [PATCH 14/21] drm/xe/uapi: Simplify OA configs in uapi X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-xe@lists.freedesktop.org Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" 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 > > Signed-off-by: Ashutosh Dixit > > lgtm > Reviewed-by: Umesh Nerlige Ramappa 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 > >