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 0093BC02180 for ; Wed, 15 Jan 2025 23:46:38 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id BEFF810E84D; Wed, 15 Jan 2025 23:46:38 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="Pvc+X5BS"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.13]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3670610E84D for ; Wed, 15 Jan 2025 23:46: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=1736984798; x=1768520798; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=qRVHqshthx9EeMykcdkCdHMJR+QjyjbcNu8EagucpN0=; b=Pvc+X5BSEtYInDIsahruDgjeDDIuykBxEhamh/olxaAY0HEE6J1tbEXV qw/J0ndzIJeuhTOQkf0TTONuctGXn1ZLcPLcAi4I9l7tsX4UJcL/wnFsz tAXeyOha8sz0/fciGQiJ7bZpSkP+MnbW2vEUgD3ErsfBuMi7C3imwAfIk NbTVEbbPo9j+SdtOGYMdpLI266rF/OfMBW+1N9Kx9Zm0dwTY26gD7b8Ez 3MazvBKWwkL0o2wsX7URSpTyDkSAk3td4m1oZ3OCjd+CvGPFMBgIoOpBE AK6oJrgNqqCPfUzUVx5mnnr7JEApeC4q9l7u4cFTHxJgh0v76k78r219K Q==; X-CSE-ConnectionGUID: aWB1uHKGSN+A6O1BAtS1cw== X-CSE-MsgGUID: +qM0F0CHRc2Rx5S7q52GCg== X-IronPort-AV: E=McAfee;i="6700,10204,11316"; a="40157665" X-IronPort-AV: E=Sophos;i="6.13,207,1732608000"; d="scan'208";a="40157665" Received: from fmviesa003.fm.intel.com ([10.60.135.143]) by fmvoesa107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Jan 2025 15:46:38 -0800 X-CSE-ConnectionGUID: +JyfzGqzQjOnY1x5o1Od8Q== X-CSE-MsgGUID: aAEM0J6aQTS5a3UizqEmlA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.12,224,1728975600"; d="scan'208";a="109330474" Received: from orsosgc001.jf.intel.com (HELO orsosgc001.intel.com) ([10.165.21.142]) by fmviesa003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Jan 2025 15:46:37 -0800 Date: Wed, 15 Jan 2025 15:46:37 -0800 Message-ID: <85r053ej36.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: "Cavitt, Jonathan" Cc: "intel-xe@lists.freedesktop.org" , "Nerlige Ramappa, Umesh" Subject: Re: [PATCH] drm/xe/oa: Set stream->pollin in xe_oa_buffer_check_unlocked In-Reply-To: References: <20250115222029.3002103-1-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 Wed, 15 Jan 2025 14:57:24 -0800, Cavitt, Jonathan wrote: > > -----Original Message----- > From: Intel-xe On Behalf Of Ashutosh Dixit > Sent: Wednesday, January 15, 2025 2:20 PM > To: intel-xe@lists.freedesktop.org > Cc: Nerlige Ramappa, Umesh > Subject: [PATCH] drm/xe/oa: Set stream->pollin in xe_oa_buffer_check_unlocked > > > > We rely on stream->pollin to decide whether or not to block during > > poll/read calls. However, currently there are blocking read code paths > > which don't even set stream->pollin. The best place to consistently set > > stream->pollin for all code paths is therefore to set it in > > xe_oa_buffer_check_unlocked. > > > > Fixes: e936f885f1e9 ("drm/xe/oa/uapi: Expose OA stream fd") > > Signed-off-by: Ashutosh Dixit > > Minor concern below, but I don't think it's a problem, so I won't block on it. > Reviewed-by: Jonathan Cavitt > > > --- > > drivers/gpu/drm/xe/xe_oa.c | 9 +++------ > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c > > index eeb96b5f49e2a..e27c1752aa57c 100644 > > --- a/drivers/gpu/drm/xe/xe_oa.c > > +++ b/drivers/gpu/drm/xe/xe_oa.c > > @@ -237,7 +237,6 @@ static bool xe_oa_buffer_check_unlocked(struct xe_oa_stream *stream) > > u32 tail, hw_tail, partial_report_size, available; > > int report_size = stream->oa_buffer.format->size; > > unsigned long flags; > > - bool pollin; > > > > spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags); > > > > @@ -282,11 +281,11 @@ static bool xe_oa_buffer_check_unlocked(struct xe_oa_stream *stream) > > stream->oa_buffer.tail = tail; > > > > available = xe_oa_circ_diff(stream, stream->oa_buffer.tail, stream->oa_buffer.head); > > - pollin = available >= stream->wait_num_reports * report_size; > > + stream->pollin = available >= stream->wait_num_reports * report_size; > > It looks like we call xe_oa_buffer_check_unlocked in two places. > > The first is below, where we call it during xe_oa_poll_check_timer_cb. I see we're > operating on the stream->pollin value here because we're migrating the > "stream->polling = true" code from xe_oa_poll_check_timer_cb into the > xe_oa_buffer_check_unlocked function. > > However, the second location is during xe_oa_wait_unlocked. There, we perform > xe_oa_buffer_check_unlocked in a wait_event_interruptible loop. Is it safe to be > modifying the stream->pollin value in that case? > > I'm sure it's fine, but it's something to consider. Yes it's fine. Actually it is one of the cases where stream->pollin is not set (when wait_event_interruptible does *not* block). Because currently we are setting stream->pollin only when OA data is detected from the timer callback (when we are blocked and unblocked by the timer). But we should really be setting stream->pollin in all cases (i.e. also in those cases where we don't block), which is what this patch is doing. So one of those cases where stream->pollin is not set is when wait_event_interruptible does not block. Another potential case is what Umesh is thinking about and might post. Thanks, Ashutosh > > > > spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags); > > > > - return pollin; > > + return stream->pollin; > > } > > > > static enum hrtimer_restart xe_oa_poll_check_timer_cb(struct hrtimer *hrtimer) > > @@ -294,10 +293,8 @@ static enum hrtimer_restart xe_oa_poll_check_timer_cb(struct hrtimer *hrtimer) > > struct xe_oa_stream *stream = > > container_of(hrtimer, typeof(*stream), poll_check_timer); > > > > - if (xe_oa_buffer_check_unlocked(stream)) { > > - stream->pollin = true; > > + if (xe_oa_buffer_check_unlocked(stream)) > > wake_up(&stream->poll_wq); > > - } > > > > hrtimer_forward_now(hrtimer, ns_to_ktime(stream->poll_period_ns)); > > > > -- > > 2.47.1 > > > >