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 D0B65C678D5 for ; Sat, 11 Mar 2023 03:13:46 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 73B9E10E02B; Sat, 11 Mar 2023 03:13:45 +0000 (UTC) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0F4BE10E02B for ; Sat, 11 Mar 2023 03:13:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1678504424; x=1710040424; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=UNUmsBfcyKJSV9d7mUPU08Y2A19/XUk/qf52r2gjdRg=; b=S09eFwLM1QFcXPYD9L2pmiw3m67w9uXiPO8/C4JAPvsGPrgJqzQ28IvJ VjI6OraEyxN0UAHCpYsJEy/FKNJmOi8DDbRtbraKOIbvcz2AhS/W7a/nw Vquey1EmpZAZNXv7c5g+Sc+nbiADlx8FqrGVEm2aIJYQPLoB87J3Rvb7Z hiV/kgSO7WZn2rSFiEskJFXlW6J+8AQt1sN6XeB2jTQ+Me+1uozsBmcEw Aa+CQniELpJDrd4j/uinW747Werd7tRPnomZQium6LV3l5edL7BdXLHIH gtwgksXAEiCOjNXhonRQcFUMAB/ZP4UAHTv+mSIDTTsmYXyiglBuJKOY2 Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10645"; a="401733254" X-IronPort-AV: E=Sophos;i="5.98,251,1673942400"; d="scan'208";a="401733254" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Mar 2023 19:13:42 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10645"; a="708241222" X-IronPort-AV: E=Sophos;i="5.98,251,1673942400"; d="scan'208";a="708241222" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.251.14.229]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Mar 2023 19:13:42 -0800 Date: Fri, 10 Mar 2023 19:13:42 -0800 Message-ID: <87y1o4kmjd.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa In-Reply-To: References: <20230307201611.773103-1-umesh.nerlige.ramappa@intel.com> <20230307201611.773103-10-umesh.nerlige.ramappa@intel.com> <87a60lmq9v.wl-ashutosh.dixit@intel.com> <87zg8kld8r.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/28.2 (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-gfx] [PATCH v4 9/9] drm/i915/perf: Add support for OA media units X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Fri, 10 Mar 2023 16:18:30 -0800, Umesh Nerlige Ramappa wrote: > > On Fri, Mar 10, 2023 at 09:36:52AM -0800, Dixit, Ashutosh wrote: > > On Fri, 10 Mar 2023 08:39:27 -0800, Umesh Nerlige Ramappa wrote: > >> > > > > Hi Umesh, > > > >> On Thu, Mar 09, 2023 at 03:57:48PM -0800, Dixit, Ashutosh wrote: > >> > On Tue, 07 Mar 2023 12:16:11 -0800, Umesh Nerlige Ramappa wrote: > >> >> > >> >> -static int gen8_configure_context(struct i915_gem_context *ctx, > >> >> +static int gen8_configure_context(struct i915_perf_stream *stream, > >> >> + struct i915_gem_context *ctx, > >> >> struct flex *flex, unsigned int count) > >> >> { > >> >> struct i915_gem_engines_iter it; > >> >> @@ -2573,7 +2594,8 @@ static int gen8_configure_context(struct i915_gem_context *ctx, > >> >> for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) { > >> >> GEM_BUG_ON(ce == ce->engine->kernel_context); > >> >> > >> >> - if (!engine_supports_oa(ce->engine)) > >> >> + if (!engine_supports_oa(ce->engine) || > >> >> + ce->engine->class != stream->engine->class) > >> >> continue; > >> >> > >> >> /* Otherwise OA settings will be set upon first use */ > >> >> @@ -2704,7 +2726,7 @@ oa_configure_all_contexts(struct i915_perf_stream *stream, > >> >> > >> >> spin_unlock(&i915->gem.contexts.lock); > >> >> > >> >> - err = gen8_configure_context(ctx, regs, num_regs); > >> >> + err = gen8_configure_context(stream, ctx, regs, num_regs); > >> >> if (err) { > >> >> i915_gem_context_put(ctx); > >> >> return err; > >> >> @@ -2724,7 +2746,8 @@ oa_configure_all_contexts(struct i915_perf_stream *stream, > >> >> for_each_uabi_engine(engine, i915) { > >> >> struct intel_context *ce = engine->kernel_context; > >> >> > >> >> - if (!engine_supports_oa(ce->engine)) > >> >> + if (!engine_supports_oa(ce->engine) || > >> >> + ce->engine->class != stream->engine->class) > >> >> continue; > >> >> > >> >> regs[0].value = intel_sseu_make_rpcs(engine->gt, &ce->sseu); > >> >> @@ -2749,6 +2772,9 @@ gen12_configure_all_contexts(struct i915_perf_stream *stream, > >> >> }, > >> >> }; > >> >> > >> >> + if (stream->engine->class != RENDER_CLASS) > >> >> + return 0; > >> >> + > >> >> return oa_configure_all_contexts(stream, > >> >> regs, ARRAY_SIZE(regs), > >> >> active); > >> > > >> > Can you please explain the above changes? Why are we checking for > >> > engine->class above? Should we be checking for both class and instance? Or > >> > all engines connected to an OA unit (multiple classes can be connected to > >> > an OA unit and be different from stream->engine->class, e.g. VDBOX and > >> > VEBOX)? oa_configure_all_contexts is also called from > >> > lrc_configure_all_contexts. > > This check primarily blocks media engine use cases from entering > oa_configure_all_contexts(). > > lrc_configure_all_contexts applies to pre-gen12 only. On pre-gen12, > engine_supports_oa() should return true only for render. > >> > >> Only render (and compute when we support it) have OA specific configuration > >> in the context image. Media engines do not have any context specific > >> configurations. > > > > Yes I remember you answered this previously too. My question still is why > > did we make the 2 instances of this change above: > > > > From the original code in drm-tip: > > > > if (engine->class != RENDER_CLASS) > > continue; > > > > To the final code (changed in two patches): > > > > if (!engine_supports_oa(ce->engine) || > > ce->engine->class != stream->engine->class) > > continue; > > I think some changes are a result of incrementally supporting compute and > then media in OA. Since we have not upstreamed the compute support, some > lines of code remain. > > With compute support the "if (engine->class != RENDER_CLASS)" changed to > "if (!engine_supports_oa(ce->engine)). Later, OAM support brought the other > condition that checks classes because this code is under > for_each_uabi_engine(engine, i915). When we run this for an OA use case > where user has passed rcs0 for ex, it will still iterate over the media > engines. Since we now support media engines, we should skip them in this > loop. > The other question on whether this should be class specific or span > multiple engines, I have to check that specifically for OAG. Ideally, the > PWR_CLK_STATE should be configured for all engines that support it (render > and compute where available), so the above check should be > if (!engine_supports_oa(ce->engine) || > !engine_has_pwr_clk_state(ce->engine)) > > A jira will help track this and I can address that in a separate > patch/series if it turns out to be an issue. OK, this is my last question on this patch, everything else is fine. Since you will post another version of the series I will review your (hopefully final) changes about the above segment there and let's go from there. Thanks. -- Ashutosh