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 BF0E6C52D7C for ; Wed, 21 Aug 2024 15:30:07 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8D37B10E416; Wed, 21 Aug 2024 15:30:07 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="MJCqBgyu"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.14]) by gabe.freedesktop.org (Postfix) with ESMTPS id 644FD10E416 for ; Wed, 21 Aug 2024 15:30:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1724254206; x=1755790206; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=xS1paI8bWDWOvClvYkyLDT0MqbTEKDRX0wIu+jDGW54=; b=MJCqBgyup2V9+VMHAv4x4+dJa7eYJ9KJGKxdg0BVbpKD9Zb28j/+JTBA mrsQtX3mokSgbJNEUAx0zXxk5DTXRzI/LoKmGDybA0lNEsTlYr4bvg8iO p2BxwPI3Ql+m2F6NAY2tBFEPTo0dYCE+7Xm6vpZyRHMOvgE4rbjcYtFNh lOac/HKy4CeqjM94j6nl7dMtTDZ1YIXYzSUkPfpv6+pukMOpN1qwZp4/3 jXguK/+hRuksOIAD+8RH7c8eW2EW34zuL1OjnNnjrhxg8gwCN5pvTOsXf tOlDhJ21CLoCve9+ElYJZQT71VBAkSfc6419WTkjy7oX3YSUCO16Us6aX w==; X-CSE-ConnectionGUID: iLKEGZgiRwyhgC54THmaiA== X-CSE-MsgGUID: VkD0VnajRXOjEh+tW2ysNQ== X-IronPort-AV: E=McAfee;i="6700,10204,11171"; a="22806960" X-IronPort-AV: E=Sophos;i="6.10,164,1719903600"; d="scan'208";a="22806960" Received: from fmviesa010.fm.intel.com ([10.60.135.150]) by fmvoesa108.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Aug 2024 08:30:06 -0700 X-CSE-ConnectionGUID: dApSC6RpTUO9YV512AdVUQ== X-CSE-MsgGUID: cmiVOGWbT8e858DzaRV32Q== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.10,164,1719903600"; d="scan'208";a="61259613" Received: from jorgera1-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.124.118.77]) by fmviesa010-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Aug 2024 08:30:06 -0700 Date: Wed, 21 Aug 2024 08:19:56 -0700 Message-ID: <877cc99a3n.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: "Cavitt, Jonathan" Cc: "intel-xe@lists.freedesktop.org" , "Brost, Matthew" , "Souza, Jose" , "Landwerlin, Lionel G" , "Nerlige Ramappa, Umesh" Subject: Re: [PATCH 7/7] drm/xe/oa: Allow only certain property changes from config In-Reply-To: References: <20240820005808.1412649-1-ashutosh.dixit@intel.com> <20240820005808.1412649-8-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.4 (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 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: , Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Tue, 20 Aug 2024 07:15:51 -0700, Cavitt, Jonathan wrote: > Hi Jonathan, > -----Original Message----- > From: Dixit, Ashutosh > Sent: Monday, August 19, 2024 5:58 PM > To: intel-xe@lists.freedesktop.org > Cc: Brost, Matthew ; Souza, Jose ; Landwerlin, Lionel G ; Nerlige Ramappa, Umesh ; Cavitt, Jonathan > Subject: [PATCH 7/7] drm/xe/oa: Allow only certain property changes from config > > > > Whereas all properties can be specified during OA stream open, when the OA > > stream is reconfigured only the config_id and syncs can be specified. > > > > v2: Use separate function table for reconfig case (Jonathan) > > Change bool function args to enum (Matt B) > > > > Signed-off-by: Ashutosh Dixit > > > Minor nits below, but otherwise LGTM. > Reviewed-by: Jonathan Cavitt Thanks! > > > --- > > drivers/gpu/drm/xe/xe_oa.c | 51 ++++++++++++++++++++++++++++++-------- > > 1 file changed, 40 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c > > index f43767e0a75ec..96420c3ea10fd 100644 > > --- a/drivers/gpu/drm/xe/xe_oa.c > > +++ b/drivers/gpu/drm/xe/xe_oa.c > > @@ -47,6 +47,11 @@ enum xe_oa_submit_deps { > > XE_OA_SUBMIT_ADD_DEPS, > > }; > > > > +enum xe_oa_user_extn_from { > > + XE_OA_USER_EXTN_FROM_OPEN, > > + XE_OA_USER_EXTN_FROM_CONFIG, > > +}; > > + > > struct xe_oa_reg { > > struct xe_reg addr; > > u32 value; > > @@ -1246,6 +1251,12 @@ static int xe_oa_set_prop_syncs_user(struct xe_oa *oa, u64 value, > > return 0; > > } > > > > +static int xe_oa_set_prop_ret_inval(struct xe_oa *oa, u64 value, > > + struct xe_oa_open_param *param) > > +{ > > + return -EINVAL; > > +} > > + > > typedef int (*xe_oa_set_property_fn)(struct xe_oa *oa, u64 value, > > struct xe_oa_open_param *param); > > static const xe_oa_set_property_fn xe_oa_set_property_funcs[] = { > > > Perhaps we should consider renaming this to match the later funcs array. > "xe_oa_set_property_funcs_open" for example? > Just a suggestion, not blocking. OK I went ahead and made this change in v3. Increases the patch footprint a bit but makes it clearer. > > > > @@ -1262,8 +1273,22 @@ static const xe_oa_set_property_fn xe_oa_set_property_funcs[] = { > > [DRM_XE_OA_PROPERTY_SYNCS] = xe_oa_set_prop_syncs_user, > > }; > > > > -static int xe_oa_user_ext_set_property(struct xe_oa *oa, u64 extension, > > - struct xe_oa_open_param *param) > > +static const xe_oa_set_property_fn xe_oa_set_property_funcs_config[] = { > > + [DRM_XE_OA_PROPERTY_OA_UNIT_ID] = xe_oa_set_prop_ret_inval, > > + [DRM_XE_OA_PROPERTY_SAMPLE_OA] = xe_oa_set_prop_ret_inval, > > + [DRM_XE_OA_PROPERTY_OA_METRIC_SET] = xe_oa_set_prop_metric_set, > > + [DRM_XE_OA_PROPERTY_OA_FORMAT] = xe_oa_set_prop_ret_inval, > > + [DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT] = xe_oa_set_prop_ret_inval, > > + [DRM_XE_OA_PROPERTY_OA_DISABLED] = xe_oa_set_prop_ret_inval, > > + [DRM_XE_OA_PROPERTY_EXEC_QUEUE_ID] = xe_oa_set_prop_ret_inval, > > + [DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE] = xe_oa_set_prop_ret_inval, > > + [DRM_XE_OA_PROPERTY_NO_PREEMPT] = xe_oa_set_prop_ret_inval, > > + [DRM_XE_OA_PROPERTY_NUM_SYNCS] = xe_oa_set_prop_num_syncs, > > + [DRM_XE_OA_PROPERTY_SYNCS] = xe_oa_set_prop_syncs_user, > > +}; > > + > > +static int xe_oa_user_ext_set_property(struct xe_oa *oa, enum xe_oa_user_extn_from from, > > + u64 extension, struct xe_oa_open_param *param) > > { > > u64 __user *address = u64_to_user_ptr(extension); > > struct drm_xe_ext_set_property ext; > > @@ -1279,18 +1304,22 @@ static int xe_oa_user_ext_set_property(struct xe_oa *oa, u64 extension, > > return -EINVAL; > > > > idx = array_index_nospec(ext.property, ARRAY_SIZE(xe_oa_set_property_funcs)); > > - return xe_oa_set_property_funcs[idx](oa, ext.value, param); > > + > > + if (from == XE_OA_USER_EXTN_FROM_CONFIG) > > + return xe_oa_set_property_funcs_config[idx](oa, ext.value, param); > > + else > > + return xe_oa_set_property_funcs[idx](oa, ext.value, param); > > > If we want to extend this in the future, we might want a map between the > xe_oa_user_extn_from enum and the xe_oa_set_property_funcs_config. > > Something like: > > """ > static const xe_oa_set_property_fn xe_oa_set_property_funcs_list[][] { > [XE_OA_USER_EXTN_FROM_OPEN] = xe_oa_set_property_funcs, > [XE_OA_USER_EXTN_FROM_CONFIG] = xe_oa_set_property_funcs_config, > }; > """ > > Then, here, we'd just need to do something like: > > """ > return xe_oa_set_property_funcs_list[from][idx](oa, ext.value, param); > """ > > I don't think this is strictly necessary, though. This one I was tempted to make too, but then I let it be. I don't think we are expecting more code paths where we expect this to happen. But thanks for the suggestion, it is a good one. Thanks. -- Ashutosh > > } > > > > -typedef int (*xe_oa_user_extension_fn)(struct xe_oa *oa, u64 extension, > > - struct xe_oa_open_param *param); > > +typedef int (*xe_oa_user_extension_fn)(struct xe_oa *oa, enum xe_oa_user_extn_from from, > > + u64 extension, struct xe_oa_open_param *param); > > static const xe_oa_user_extension_fn xe_oa_user_extension_funcs[] = { > > [DRM_XE_OA_EXTENSION_SET_PROPERTY] = xe_oa_user_ext_set_property, > > }; > > > > #define MAX_USER_EXTENSIONS 16 > > -static int xe_oa_user_extensions(struct xe_oa *oa, u64 extension, int ext_number, > > - struct xe_oa_open_param *param) > > +static int xe_oa_user_extensions(struct xe_oa *oa, enum xe_oa_user_extn_from from, u64 extension, > > + int ext_number, struct xe_oa_open_param *param) > > { > > u64 __user *address = u64_to_user_ptr(extension); > > struct drm_xe_user_extension ext; > > @@ -1309,12 +1338,12 @@ static int xe_oa_user_extensions(struct xe_oa *oa, u64 extension, int ext_number > > return -EINVAL; > > > > idx = array_index_nospec(ext.name, ARRAY_SIZE(xe_oa_user_extension_funcs)); > > - err = xe_oa_user_extension_funcs[idx](oa, extension, param); > > + err = xe_oa_user_extension_funcs[idx](oa, from, extension, param); > > if (XE_IOCTL_DBG(oa->xe, err)) > > return err; > > > > if (ext.next_extension) > > - return xe_oa_user_extensions(oa, ext.next_extension, ++ext_number, param); > > + return xe_oa_user_extensions(oa, from, ext.next_extension, ++ext_number, param); > > > > return 0; > > } > > @@ -1460,7 +1489,7 @@ static long xe_oa_config_locked(struct xe_oa_stream *stream, u64 arg) > > struct xe_oa_config *config; > > int err; > > > > - err = xe_oa_user_extensions(stream->oa, arg, 0, ¶m); > > + err = xe_oa_user_extensions(stream->oa, XE_OA_USER_EXTN_FROM_CONFIG, arg, 0, ¶m); > > if (err) > > return err; > > > > @@ -2011,7 +2040,7 @@ int xe_oa_stream_open_ioctl(struct drm_device *dev, u64 data, struct drm_file *f > > } > > > > param.xef = xef; > > - ret = xe_oa_user_extensions(oa, data, 0, ¶m); > > + ret = xe_oa_user_extensions(oa, XE_OA_USER_EXTN_FROM_OPEN, data, 0, ¶m); > > if (ret) > > return ret; > > > > -- > > 2.41.0 > > > >