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 9A4B91088E72 for ; Thu, 19 Mar 2026 03:55:47 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 4E43210E0C5; Thu, 19 Mar 2026 03:55:47 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="cQS05xrT"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.7]) by gabe.freedesktop.org (Postfix) with ESMTPS id A27D210E0C5 for ; Thu, 19 Mar 2026 03:55:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1773892545; x=1805428545; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=siJ0GOMZsGploVUym2mBbLk60+a3H3uMi2mPDROxIkc=; b=cQS05xrToVAtZtz/2Uv29C0b3fiq0pgNxihcLf+y986Oh4lECOb/z/7/ EBXJAS04jchr3R/960lwb0or13C1IgTZS8rvMEz/i+MYY3Qj/frO7aaxD OHby8YEt88VT/kaom21tI5vnoc3+8HaWgb7jTueN1UYs++rtobcX44yM9 eQRBfQt+nO8yhkZaxN7/8ISOjRDxbetNPoube6jQVrdKhSvwRvUHMXaw6 sicZN+sSfSC1VR5voWGc981qx2krNpnWDIufyRBMLW2pJ+KjulCzfjH4j 8KHEArgA3lbYJcPtHC4WM17Lo/KRnO4WP/0W0urZkMIFh/QIVJ724nq7/ w==; X-CSE-ConnectionGUID: WHgdYnYPTmu2PBAQy8ungg== X-CSE-MsgGUID: 0tg+638NT6eD13VIj1B6hA== X-IronPort-AV: E=McAfee;i="6800,10657,11733"; a="100420534" X-IronPort-AV: E=Sophos;i="6.23,128,1770624000"; d="scan'208";a="100420534" Received: from orviesa005.jf.intel.com ([10.64.159.145]) by fmvoesa101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Mar 2026 20:55:45 -0700 X-CSE-ConnectionGUID: j1WEv6O0QSuk0FhuESadMQ== X-CSE-MsgGUID: /UgSK1eLQ2CrDlAW602NFA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.23,128,1770624000"; d="scan'208";a="227788252" Received: from btannenx-mobl.amr.corp.intel.com (HELO adixit-MOBL3.intel.com) ([10.125.35.72]) by orviesa005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Mar 2026 20:55:45 -0700 Date: Wed, 18 Mar 2026 20:55:42 -0700 Message-ID: <878qboqxq9.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Harish Chegondi Cc: , , , Subject: Re: [PATCH v2 1/1] drm/xe/eustall: Return EBADFD from read if EU stall registers get reset In-Reply-To: References: <52d991cc7e8bec514bb582717a1c42033672d4a5.1773683739.git.harish.chegondi@intel.com> <87se9xpqub.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/30.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 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 Wed, 18 Mar 2026 14:30:06 -0700, Harish Chegondi wrote: > > On Tue, Mar 17, 2026 at 11:57:32PM -0700, Dixit, Ashutosh wrote: > > On Mon, 16 Mar 2026 10:58:56 -0700, Harish Chegondi wrote: > > > > > > If a reset (GT or engine) happens during EU stall data sampling, all the > > > EU stall registers can get reset to 0. This will result in EU stall data > > > buffers' read and write pointer register values to be out of sync with > > > the cached values. This will result in read() returning invalid data. To > > > prevent this, check the value of a EU stall base register. If it is zero, > > > it indicates a reset may have happened that wiped the register to zero. > > > If this happens, return EBADFD from read() upon which the user space > > > should close the fd and open a new fd for a new EU stall data > > > collection session. > > > > > > Cc: Ashutosh Dixit > > > Signed-off-by: Harish Chegondi > > > --- > > > v2: Move base register check from read to the poll function > > > > > > drivers/gpu/drm/xe/xe_eu_stall.c | 24 +++++++++++++++++++++++- > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/xe/xe_eu_stall.c b/drivers/gpu/drm/xe/xe_eu_stall.c > > > index c34408cfd292..7e14de73a2c9 100644 > > > --- a/drivers/gpu/drm/xe/xe_eu_stall.c > > > +++ b/drivers/gpu/drm/xe/xe_eu_stall.c > > > @@ -44,6 +44,7 @@ struct per_xecore_buf { > > > struct xe_eu_stall_data_stream { > > > bool pollin; > > > bool enabled; > > > + bool reset_detected; > > > int wait_num_reports; > > > int sampling_rate_mult; > > > wait_queue_head_t poll_wq; > > > @@ -428,6 +429,17 @@ static bool eu_stall_data_buf_poll(struct xe_eu_stall_data_stream *stream) > > > set_bit(xecore, stream->data_drop.mask); > > > xecore_buf->write = write_ptr; > > > } > > > + /* If a GT or engine reset happens during EU stall sampling, > > > + * all EU stall registers get reset to 0 and the cached values of > > > + * the EU stall data buffers' read pointers are out of sync with > > > + * the register values. This causes invalid data to be returned > > > + * from read(). To prevent this, check the value of a EU stall base > > > + * register. If it is zero, there has been a reset. > > > + */ > > > > As previously discussed, the best way would have been to not have to do > > this. We would just plug into the handler for the reset message from GuC, > > rather than to implement a reset detection here (and in other places such > > as OA). But looks like if we do that, because of the way EUSS registers are > > reset, we can return bad EUSS data. So looks like there is no way around > > doing this "reset detection" here and a solution with the GuC reset handler > > would always be racy. Just for the record. > > Thanks for the summary of the previous discussion. Yes, hooking into the > GUc reset notification handler will be racy and bad EUSS data will be > returned to the user space if read() happens after the reset but before > the GuC reset notification message is processed. That's the reason for > not taking that approach. > > > > > > + if (unlikely(!xe_gt_mcr_unicast_read_any(gt, XEHPC_EUSTALL_BASE))) { > > > + stream->reset_detected = true; > > > + min_data_present = true; > > > > I don't believe we need to set 'min_data_present = true' if we are setting > > 'stream->reset_detected = true', correct? See if statement at the bottom. > > Agree. The only difference is that the if statement at the bottom will > evaluate true in the current execution of eu_stall_data_buf_poll_work_fn > if min_data_present is set to true. If min_data_present is not set to > true, the if statement will evaluate to true in the subsequent execution > of eu_stall_data_buf_poll_work_fn() which is still okay. So, yes, we > don't have to set min_data_present to true here. Will fix in the next > version. Just switch the order of the two OR operands and you don't have that issue? > > > > Also, since the write pointer itself gets reset during reset, didn't we > > want to do this register read only when the write pointer is 0 (to avoid an > > extra register read every 5 ms)? > > Good point. I have thought about reducing the number of this register > reads. The poll function reads the write pointers of all the xecores. > A reset can happen anytime the poll function is reading the write > pointers of the xecores. If the reset happens before the poll function > started reading the write pointers, all write pointers are zeros. > If the reset happens during the poll function, several write pointers > read so far can be non-zero while the rest of the pointers after reset > are all zeros. The if reset happens right after the poll function, the > write pointers can be a mix of zeros and non-zeros. > I think the only time this register read can be skipped is if the > LAST write pointer read is non-zero which means a reset did not happen > before or during the poll function. Do you agree? I thought of adding a > check to the if statement to check if the last write pointer is > non-zero, but to keep the code clean, I didn't. Also, if there are > n xecores, there will be n write pointer register reads plus one > additional base register read, which isn't too bad? Also, hoping the use > of unlikely macro would not impact the performance too much. OK, leave this as is I think. Otherwise we'll need to read the base register each time we see a zero write pointer. So it's ok, leave as is. > > > > > + } > > > mutex_unlock(&stream->xecore_buf_lock); > > > > > > return min_data_present; > > > @@ -554,6 +566,15 @@ static ssize_t xe_eu_stall_stream_read_locked(struct xe_eu_stall_data_stream *st > > > } > > > stream->data_drop.reported_to_user = false; > > > } > > > + /* If EU stall registers got reset due to a GT/engine reset, > > > + * continuing with the read() will return invalid data to > > > + * the user space. Just return -EBADFD instead. > > > + */ > > > + if (unlikely(stream->reset_detected)) { > > > + xe_gt_dbg(gt, "EU stall base register has been reset\n"); > > > + mutex_unlock(&stream->xecore_buf_lock); > > > + return -EBADFD; > > > > The other option is to return -EIO here and implement > > DRM_XE_OBSERVATION_IOCTL_STATUS and return status from that. Let me think > > some more about this. > > I think EBADFD is more appropriate errno than EIO in this case since the > fd is in a corrupted state and user has to close and re-open the fd. > Currently, the -EIO is used to indicate drop data in which case, the > user space can continue to read the data (faster) without closing the fd. OK, we can go with -EBADFD, though still thinking about it. > > > > > + } > > > > > > for_each_dss_steering(xecore, gt, group, instance) { > > > ret = xe_eu_stall_data_buf_read(stream, buf, count, &total_size, > > > @@ -692,6 +713,7 @@ static int xe_eu_stall_stream_enable(struct xe_eu_stall_data_stream *stream) > > > xecore_buf->write = write_ptr; > > > xecore_buf->read = write_ptr; > > > } > > > + stream->reset_detected = false; > > > > So after reset, if a stream is disabled and re-enabled, we expect things to > > work again and EUSS data to be correct (without re-opening a new > > stream)? > > Technically, yes, since the EU stall registers programming is done in > enable, things will work again if the stream is disabled and re-enabled. > But if the EUSS registers programming is moved into open() in the > future, things may not work by disabling and re-enabling the stream. So, > I think we suggest to the UMDs to close the stream and open a new > stream. No we don't suggest anything to UMD's. We decide what we want to do, implement and enforce it that way and then maintain that uapi. OK, then let us make sure after disable/enable, the reset_detected flag remains set. > > > > > stream->data_drop.reported_to_user = false; > > > bitmap_zero(stream->data_drop.mask, XE_MAX_DSS_FUSE_BITS); > > > > > > @@ -717,7 +739,7 @@ static void eu_stall_data_buf_poll_work_fn(struct work_struct *work) > > > container_of(work, typeof(*stream), buf_poll_work.work); > > > struct xe_gt *gt = stream->gt; > > > > > > - if (eu_stall_data_buf_poll(stream)) { > > > + if (stream->reset_detected || eu_stall_data_buf_poll(stream)) { > > > stream->pollin = true; > > > wake_up(&stream->poll_wq); > > > } > > > -- > > > 2.43.0 > > >