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 BF7D0C433FE for ; Tue, 11 Oct 2022 00:17:50 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 93F9010E756; Tue, 11 Oct 2022 00:17:49 +0000 (UTC) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7CF5D10E759 for ; Tue, 11 Oct 2022 00:17:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1665447466; x=1696983466; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=DbuPwZlhwa5Lx1AgNgxsffnPoBr7lUy2OaHaj6g3yq8=; b=F8KvWUbk64TE6n1RNKtzBtM2draFDnWPYuiQHCyCH+Ue6UhMj389k5Z8 dERoOOr7GEoVwXKZcDgIqiOBrMl5MH4GXmpxIsCExBwrwMTxb8UEjrTro 7LKRjj8kebGH50JpwfgsAM9q2lfluIz/bC4tKB3v9NTV2bN6pq1LyNhPd 6XhilF6SYrPklbNZBrDm22x3bTbxkWWNZ8ThO5haQ1aUKNrQrXEU4HMyp Q5j4f/9yf0oSa9golUWcUtYTehdd5dHqRVCPmaZ/8C72xXexX2ntEMW2I kMP9sBkn+VePoskoIcFzTUJG2WhgYmb899axGaxOMyGlC4Tg4JmLmuPuR A==; X-IronPort-AV: E=McAfee;i="6500,9779,10496"; a="291679273" X-IronPort-AV: E=Sophos;i="5.95,173,1661842800"; d="scan'208";a="291679273" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Oct 2022 17:17:45 -0700 X-IronPort-AV: E=McAfee;i="6500,9779,10496"; a="871307735" X-IronPort-AV: E=Sophos;i="5.95,173,1661842800"; d="scan'208";a="871307735" Received: from smuttava-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.212.187.14]) by fmsmga006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Oct 2022 17:17:45 -0700 Date: Mon, 10 Oct 2022 17:17:44 -0700 Message-ID: <87tu4b1adz.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa In-Reply-To: <20221010181434.513477-5-umesh.nerlige.ramappa@intel.com> References: <20221010181434.513477-1-umesh.nerlige.ramappa@intel.com> <20221010181434.513477-5-umesh.nerlige.ramappa@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 v3 04/16] 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 Mon, 10 Oct 2022 11:14:22 -0700, Umesh Nerlige Ramappa wrote: Hi Umesh, > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index cd57b5836386..b292aa39633e 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -1358,6 +1358,68 @@ static int gen12_get_render_context_id(struct i915_perf_stream *stream) > return 0; > } > > +#define valid_oactxctrl_offset(x) ((x) && (x) != U32_MAX) > +static bool oa_find_reg_in_lri(u32 *state, u32 reg, u32 *offset, u32 end) > +{ > + u32 idx = *offset; > + u32 len = min(MI_LRI_LEN(state[idx]) + idx, end); > + bool found = false; My recommendation would be to put something like this here: if (MI_LRI_LEN(state[idx]) & 0x1) drm_warn("MI_LRI instruction with odd length\n"); Because we expect the MI_LRI length to even and I am not sure what is in the context image. But maybe not needed? > @@ -2436,15 +2514,18 @@ static int gen12_configure_oar_context(struct i915_perf_stream *stream, > }, > }; > > - /* Modify the context image of pinned context with regs_context*/ > - err = intel_context_lock_pinned(ce); > - if (err) > - return err; > + /* Modify the context image of pinned context with regs_context */ > + if (valid_oactxctrl_offset(offset)) { As I said before, this check is not needed, if we didn't have valid a offset we should return error from oa_get_render_ctx_id. For if we have this check and offset is not valid, can we skip this code and will things still work? Or do we need to return an error from gen12_configure_oar_context? > + err = intel_context_lock_pinned(ce); > + if (err) > + return err; > > - err = gen8_modify_context(ce, regs_context, ARRAY_SIZE(regs_context)); > - intel_context_unlock_pinned(ce); > - if (err) > - return err; > + err = gen8_modify_context(ce, regs_context, > + ARRAY_SIZE(regs_context)); > + intel_context_unlock_pinned(ce); > + if (err) > + return err; > + } With the if statement removed or handled otherwise, this is: Reviewed-by: Ashutosh Dixit