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 AAB26E936ED for ; Thu, 5 Oct 2023 03:09:25 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2FA0010E165; Thu, 5 Oct 2023 03:09:25 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5AAAC10E165 for ; Thu, 5 Oct 2023 03:09:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1696475363; x=1728011363; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=y6HqUoExAWovct8kp3sUm7ofjeKNISwPB6dOVTVTdXo=; b=a0BK2I/7D4PcLFYnTu+qmGXuzcd05OmSyE3HXkM0HK8JV+3JLXchvPHC LyRlkHVrT5f1ZDEjH47L+PJvf1d7yljFrR45N8+ZgoEozi1RrvvXgwP3V tpa0m08pMb922Imcq8Bx6AHzYPWSCeo2nKlwiJWVNvODECXjTemNM+iFV pwFC6DQvL52H9qffiVwiTInI5+YxpRs3dcCUpL9k8udlKgDZJ7S/R5DmI qkFlKlGNTvstbBkBK86hM43Bh0Ke2XymUfbKhL3K2+VYvCpU7k+kAubpu NC/uXCvkh259pBNBVywRg5f6w5WaPaYq5x/Qig3RGnfC9572FKVutM/Gc A==; X-IronPort-AV: E=McAfee;i="6600,9927,10853"; a="469655096" X-IronPort-AV: E=Sophos;i="6.03,201,1694761200"; d="scan'208";a="469655096" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Oct 2023 20:09:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10853"; a="701477496" X-IronPort-AV: E=Sophos;i="6.03,201,1694761200"; d="scan'208";a="701477496" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.101.181]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Oct 2023 20:09:21 -0700 Date: Wed, 04 Oct 2023 20:09:21 -0700 Message-ID: <87sf6p22am.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa In-Reply-To: <87ttr522iw.wl-ashutosh.dixit@intel.com> References: <20230919161049.2307855-1-ashutosh.dixit@intel.com> <20230919161049.2307855-21-ashutosh.dixit@intel.com> <87ttr522iw.wl-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 20/21] drm/xe/uapi: Use OA unit id to identify OA unit 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 Wed, 04 Oct 2023 20:04:23 -0700, Dixit, Ashutosh wrote: > > On Wed, 04 Oct 2023 15:37:51 -0700, Umesh Nerlige Ramappa wrote: > > > > Hi Umesh, > > > On Tue, Sep 19, 2023 at 09:10:48AM -0700, Ashutosh Dixit wrote: > > > Previous uapi uses an indirect way (the engine class/instance of an engine > > > connected to an OA unit) to identify an OA unit. Replace this by directly > > > using the OA unit ID to identify the OA unit. > > > > > > With this change DRM_XE_OA_PROP_OA_ENGINE_CLASS property is not needed any > > > more and removed. DRM_XE_OA_PROP_OA_ENGINE_INSTANCE is still used with > > > DRM_XE_OA_PROP_EXEC_QUEUE_ID. > > /snip/ > > > > +static int xe_oa_assign_hwe(struct xe_oa *oa, struct xe_oa_open_properties *props) > > > +{ > > > + struct xe_gt *gt; > > > + int i, ret = 0; > > > + > > > + if (props->exec_q) { > > > + /* When we have an exec_q, get hwe from the exec_q */ > > > + for_each_gt(gt, oa->xe, i) { > > > + props->hwe = xe_gt_hw_engine(gt, props->exec_q->class, > > > + props->instance, false); > > > + if (props->hwe) > > > + break; > > > + } > > > + if (props->hwe && (xe_oa_unit_id(props->hwe) != props->oa_unit_id)) { > > > + drm_dbg(&oa->xe->drm, "OA unit ID mismatch for exec_q\n"); > > > + ret = -EINVAL; > > > + } > > > + } else { > > > + struct xe_hw_engine *hwe; > > > + enum xe_hw_engine_id id; > > > + > > > + /* Else just get the first hwe attached to the oa unit */ > > > + for_each_gt(gt, oa->xe, i) { > > > + for_each_hw_engine(hwe, gt, id) { > > > + if (xe_oa_unit_id(hwe) == props->oa_unit_id) { > > > + props->hwe = hwe; > > > + goto out; > > > + } > > > + } > > > + } > > > + } > > > > I think both the if and else blocks above will get you the same hwe object, > > so you can just pick the else code to assign hwe. > > Not when there are multiple hwe's attached to a single OA unit (I am > thinking media/compute), correct? > > > Are we allowing the user to pass either oa_unit_id OR exec_q/instance? > > No. oa_unit_id is the basic entity used to identify the OA unit. If > userland is only using OAG they can skip exec_q. If exec_q is specified, > the code above checks if exec_q/instance lands on the same OA unit as the > oa_unit_id. > > So oa_unit_id is always provided (for both OAG/OAR) and optionally > exec_q/instance can be provided (for OAR). > > i915 had basically and this XE uapi has > so the semantics here is almost the same as > i915. > > > I think based on the HW requirement we should enable OAR/OAG together, so > > maybe enforce that the user passes both params. Thoughts? > > I don't think it will fly since in some cases user is only interested in > OAG (global data for all contexts) so no point forcing them to also passing > in exec_q/instance. The user may not even have a specific exec_q in such > case to pass into the uapi. There are some additional considerations for the future about this: I think we should really not need exec_q_id in the OA uapi. We should be able to enable OAR functionality on all exec_q/instance combos (both which are present and which may be created while OA is enabled) which can be scheduled on that OAR unit. This would enable an OA MI_RPC query to be issued on any exec queue which exists while the OA stream is open. OAR registers are part of the context image so this looks feasible to me. The instance field is interesting since I believe compute has a limitation that OAC can only be enabled on a single hwe at a time. So even if we don't have an exec_q, instance can be used to identify which hwe instance we are enabling OAC on. So the semantics of instance will change in this case: at present instance is the context image instance on which OAR/OAC is enabled, later it will reflect the HW limitation of the engine instance. Though we may not be do this immediately because of other priorities. So currently we do single exec_q but can deprecate that if we can ever get "any exec_q" to work. Something like that (not completely thought through yet). Let's not worry about this now but just wanted to write out my thoughts about this in case you had any feedback. Thanks. -- Ashutosh