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 682FFC0218D for ; Wed, 29 Jan 2025 04:32:43 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 230C010E2A9; Wed, 29 Jan 2025 04:32:43 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="mb+JH6gP"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.17]) by gabe.freedesktop.org (Postfix) with ESMTPS id C17EC10E2A9 for ; Wed, 29 Jan 2025 04:32:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1738125162; x=1769661162; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=pGLS0sb6z6Dr9c25uaMV2SCFWjegp8CZxo7yECZx3ho=; b=mb+JH6gPNggfJspQE/P2vYwPNxq+oiOo16HVtUxjyotYVK3bEDRFqlum 1yk/I/QW8JHPHnkfZIxnH4sTgoh9Bklb1J/RlXfWcGY/TtH+AuJ7hps3B ao/e8nulzE8dsFpUEvsGi35kTj6dEjE+mCkLX5z0HQXfPnSrgzI4oITMH EsL0P37XUaysO+U522mLKOsn5PUYvsV1TeBcbNk/QUcalxJUSBa6QocTp 6D2TgnXqdIppu/keFiZyd5p/z/xtg6lEuSXTi+v5K/RSqh3sd8W6u/cfk UAOXVqCrNds/CRfAynYpv8ZxTbQM/5YFzilnK+bviIFTvsL1oPi3fEYA5 w==; X-CSE-ConnectionGUID: dd2Z1rolSXORbnINJHrIZw== X-CSE-MsgGUID: t5xWggWDSYmW617iFsMslQ== X-IronPort-AV: E=McAfee;i="6700,10204,11329"; a="38661135" X-IronPort-AV: E=Sophos;i="6.13,242,1732608000"; d="scan'208";a="38661135" Received: from fmviesa001.fm.intel.com ([10.60.135.141]) by orvoesa109.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jan 2025 20:32:41 -0800 X-CSE-ConnectionGUID: 1oV2zrbFT92w9YEiHCoq2g== X-CSE-MsgGUID: 8w1rcq5uSbydCVqhr16yeA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.12,224,1728975600"; d="scan'208";a="139795250" Received: from orsosgc001.jf.intel.com (HELO orsosgc001.intel.com) ([10.165.21.142]) by smtpauth.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jan 2025 20:32:41 -0800 Date: Tue, 28 Jan 2025 20:32:40 -0800 Message-ID: <858qqufdef.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Harish Chegondi Cc: Subject: Re: [PATCH v8 4 3/7] drm/xe/eustall: Implement EU stall sampling APIs for Xe_HPC In-Reply-To: <85a5bafec6.wl-ashutosh.dixit@intel.com> References: <28e9b33c1fd5164d611d1ea3caa45388278efe43.1736970203.git.harish.chegondi@intel.com> <85a5bafec6.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-redhat-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 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: , Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Tue, 28 Jan 2025 20:12:25 -0800, Dixit, Ashutosh wrote: > > On Wed, 15 Jan 2025 12:02:09 -0800, Harish Chegondi wrote: > > > > Hi Harish, > > Reveiw #4 on the same patch. Final review on this version of the patch. I > have suggested some low level code changes which should work but would need > to be verified (the code, as well as tested). > > Also, I don't understand really what's going on in these circ buffer > functions with the overflow bits. So I need some explanation as to what the > code is doing and whether what's going on here is really correct. > > So basically what I don't understand here is whether: > > if (write_offset > read_offset) > > should really be > > if (write_offset >= read_offset) > > Basically what happens when 'write_offset == read_offset'. We should be > using the overflow bits "somehow" according to me in this case but we don't > seem to be doing that. > > This is repeated several times in my comments below. But if you have an > explanation just explain it here, don't have to repeat it each time. Let's have one or two centralized functions to do this everywhere rather than compute read/write offsets everywhere. For example a correct buf_data_size() function should return how many bytes are in the buffer, taking into account whether the buffer is empty or full. And then this function will be called everywhere where we need to do this. And we can have more than one such centralized function. Similar to xe_oa_circ_diff and xe_oa_circ_incr for OA. > > +static u32 > > +buf_data_size(size_t buf_size, u32 read_ptr, u32 write_ptr) > > +{ > > + u32 read_offset, write_offset, size = 0; > > + > > + read_offset = read_ptr & (buf_size - 1); > > + write_offset = write_ptr & (buf_size - 1); > > + > > + if (write_offset > read_offset)