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 9D309C433FE for ; Fri, 30 Sep 2022 23:09:41 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 95F9D10E221; Fri, 30 Sep 2022 23:09:40 +0000 (UTC) Received: from mga06.intel.com (mga06b.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 563DD10E221 for ; Fri, 30 Sep 2022 23:09:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1664579378; x=1696115378; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=FNh5IYWYozXEQ/szylrf5t6KyoduxGPlPAveL5bS/10=; b=i4f+GhBID6bynJ2jvq7yxpZ41QKzpPz7qxXY+3qaYwgzhG/dXeMbRyMM NqZB9dtumCjTG3uqGjdixr9pks7KpOm+xKrgRqXU56E6RhBwmqVtSMbr9 oEIjLJVKTSYIh9A8LiA0N0QIesWLbVCoW1EapKh+/6p59b7RMvFuGY/I+ groKF4dUAO1N7tjRtrrmO97DfMbVJSNKpHXhjImajT0Fu/7Ri4FHJfZtA pN8gpdJDkCPGdj7y30NaOZIWV21wyqZmG6SoNmlxNzzPthJHByZNpj6fD 1lZsBP6DTWxJaPBA/vTHi6V5D9OvEBuS097ERnnj9J3cpHDJEHen5HRAD Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10486"; a="364154050" X-IronPort-AV: E=Sophos;i="5.93,359,1654585200"; d="scan'208";a="364154050" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Sep 2022 16:09:37 -0700 X-IronPort-AV: E=McAfee;i="6500,9779,10486"; a="765293010" X-IronPort-AV: E=Sophos;i="5.93,359,1654585200"; d="scan'208";a="765293010" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.251.25.117]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Sep 2022 16:09:36 -0700 Date: Fri, 30 Sep 2022 16:09:36 -0700 Message-ID: <87leq0zcgf.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa In-Reply-To: References: <20220923201154.283784-1-umesh.nerlige.ramappa@intel.com> <20220923201154.283784-5-umesh.nerlige.ramappa@intel.com> <87y1u45rla.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.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-gfx] [PATCH v2 04/15] drm/i915/perf: Determine gen12 oa ctx offset at runtime 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, 30 Sep 2022 14:42:07 -0700, Umesh Nerlige Ramappa wrote: > > >> +static int __set_oa_ctx_ctrl_offset(struct intel_context *ce) > > > > I have seen people complain about unnecessary double underscores in front > > of function names ;-) > > will remove/change to oa_*. > > > > >> +{ > >> + i915_reg_t reg = GEN12_OACTXCONTROL(ce->engine->mmio_base); > >> + struct i915_perf *perf = &ce->engine->i915->perf; > >> + u32 saved_offset = perf->ctx_oactxctrl_offset; > >> + u32 offset; > >> + > >> + /* Do this only once. Failure is stored as offset of U32_MAX */ > >> + if (saved_offset) > >> + return 0; > > > > But if saved_offset is U32_MAX we should be returning -ENODEV? > > correct, the above if block should be: > > if (__valid_oactxctrl_offset(offset)) > return 0; > > if (saved_offset == U32_MAX) > return -ENODEV; I would just do: u32 offset = perf->ctx_oactxctrl_offset; if (offset) goto exit; ... exit: return __valid_oactxctrl_offset(offset) ? 0 : -ENODEV; } > > > > >> + > >> + offset = __context_image_offset(ce, i915_mmio_reg_offset(reg)); > >> + perf->ctx_oactxctrl_offset = offset; > >> + > >> + drm_dbg(&ce->engine->i915->drm, > >> + "%s oa ctx control at 0x%08x dword offset\n", > >> + ce->engine->name, offset); > >> + > >> + return __valid_oactxctrl_offset(offset) ? 0 : -ENODEV; > >> +} > >> + > >> +static bool engine_supports_mi_query(struct intel_engine_cs *engine) > >> +{ > >> + return engine->class == RENDER_CLASS; > >> +} > >> + > >> /** > >> * oa_get_render_ctx_id - determine and hold ctx hw id > >> * @stream: An i915-perf stream opened for OA metrics > >> @@ -1377,6 +1435,17 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream) > >> if (IS_ERR(ce)) > >> return PTR_ERR(ce); > >> > >> + if (engine_supports_mi_query(stream->engine)) { > >> + ret = __set_oa_ctx_ctrl_offset(ce); > >> + if (ret && !(stream->sample_flags & SAMPLE_OA_REPORT)) { > > > > This is not a problem in SAMPLE_OA_REPORT case? > > SAMPLE_OA_REPORT is OAG use case. > > Actually, I did not know how to treat this condition. The current interface > will configure both OAR and OAG. If we have an error configuring OAR, > should we fail or let the OAG use case work? > > I am now leaning towards failing this unconditionally. Thoughts? Sorry I didn't follow. What does the oa_ctx_ctrl_offset in the context image do or control? Looks like oa_ctx_ctrl register controls the OA HW timer which dumps data into the OA buffer so should be programmed correctly for OAG (and possibly also for OAR). So maybe letting this fail unconditionally is correct? But I am not sure. Thanks. -- Ashutosh