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 06995C6FA82 for ; Wed, 14 Sep 2022 19:07:08 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0973A10E0E7; Wed, 14 Sep 2022 19:07:08 +0000 (UTC) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0514510E0E7 for ; Wed, 14 Sep 2022 19:07:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1663182425; x=1694718425; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=hhC1CMJD+ND0WL+lxAQrmaVrujpX5zHRCzqOQDYla9M=; b=Dav46ZUKt8wRDzW17OrmuLQFMaihdp1p7bMxGCEm2/EtIwmVdZySnRRY lytlBAy+ky5ezCwWNxSOUeV6+Uki/6Ps2HNmq6PVxfiRk5i/8RTTuOb01 1j7DKMGEqjRy2Ub4mDxpf83dYm3iUuWW59IA/Bi0lj74sHZ/AYp4pbGDd ghdaOs1YvTYxuQmZLfppKgysar86U5x9hQi8cCct38w2L08meE6tCMsiW Rb+4f2v//oYDnga58l+2jxmlp+jIDWxP1+A4UgW6/QB5WWMGtorOPfYsq 5PT70lWEuW+zERhKEt2x3umJtFk6TXVU1JcGeTwRW76agVrSAEDokKnII A==; X-IronPort-AV: E=McAfee;i="6500,9779,10470"; a="299878228" X-IronPort-AV: E=Sophos;i="5.93,315,1654585200"; d="scan'208";a="299878228" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2022 12:07:04 -0700 X-IronPort-AV: E=Sophos;i="5.93,315,1654585200"; d="scan'208";a="720693271" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.38.224]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2022 12:07:03 -0700 Date: Wed, 14 Sep 2022 12:07:03 -0700 Message-ID: <87mtb14vxk.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa In-Reply-To: References: <20220823204155.8178-1-umesh.nerlige.ramappa@intel.com> <20220823204155.8178-7-umesh.nerlige.ramappa@intel.com> <87mtb2djt1.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 06/19] drm/i915/perf: Use helpers to process reports w.r.t. OA buffer size 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 Wed, 14 Sep 2022 11:19:30 -0700, Umesh Nerlige Ramappa wrote: > > On Wed, Sep 14, 2022 at 09:04:10AM -0700, Dixit, Ashutosh wrote: > > On Tue, 23 Aug 2022 13:41:42 -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 6fc4f0d8fc5a..bbf1c574f393 100644 > >> --- a/drivers/gpu/drm/i915/i915_perf.c > >> +++ b/drivers/gpu/drm/i915/i915_perf.c > >> @@ -385,6 +385,21 @@ static struct ctl_table_header *sysctl_header; > >> > >> static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer); > >> > >> +static u32 _oa_taken(struct i915_perf_stream * stream, u32 tail, u32 head) > > > > nit: no space between * and stream. > > > >> +{ > >> + u32 size = stream->oa_buffer.vma->size; > >> + > >> + return tail >= head ? tail - head : size - (head - tail); > >> +} > > > > If we are doing this we should probably eliminate references to OA_TAKEN > > which serves an identical purpose (I think there is one remaining > > reference) and also delete OA_TAKEN #define. > > > >> + > >> +static u32 _rewind_tail(struct i915_perf_stream * stream, u32 relative_hw_tail, > >> + u32 rewind_delta) > >> +{ > >> + return rewind_delta > relative_hw_tail ? > >> + stream->oa_buffer.vma->size - (rewind_delta - relative_hw_tail) : > >> + relative_hw_tail - rewind_delta; > >> +} > > > > Also are we really saying here that we are supporting non-power-of-2 OA > > buffer sizes? Because if we stayed with power-of-2 sizes the expression > > above are nice and elegant and actually closer to the previous code being > > changed in this patch. For example: > > > > #include > > > > static u32 _oa_taken(struct i915_perf_stream *stream, u32 tail, u32 head) > > { > > return CIRC_CNT(tail, head, stream->oa_buffer.vma->size); > > } > > > > static u32 _rewind_tail(struct i915_perf_stream *stream, u32 relative_hw_tail, > > u32 rewind_delta) > > { > > return CIRC_CNT(relative_hw_tail, rewind_delta, stream->oa_buffer.vma->size); > > } > > > > Note that for power-of-2 sizes the two functions above are identical but we > > should keep them separate for clarity (as is done in the patch) since they > > are serving two different functions in the OA code. > > > > Also another assumption in the code seems to be: > > > > stream->oa_buffer.vma->size == OA_BUFFER_SIZE > > > > which I am pretty sure will not hold for arbitrary non-power-of-2 OA buffer > > sizes? So we might as well stick with power-of-2 sizes and change later in > > a separate patch only if needed? > > Most changes here are related to the OA buffer size issue and that is > specific to xehpsdv where the size is not a power of 2. I am thinking of > dropping these changes in the next revision since DG2 is fixed and OA > buffer sizes are power of 2. In the code stream->oa_buffer.vma->size and OA_BUFFER_SIZE are both used, if we want to clean that up and only use stream->oa_buffer.vma->size, we could still do soemthing like I suggested with just power-of-2 sizes and keep this patch. If we ever have to support non-power-of-2 sizes in the future we'll just need to change _oa_taken and _rewind_tail functions. Anyway your call. Thanks. -- Ashutosh