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 C4D1EC021AA for ; Wed, 19 Feb 2025 18:15:55 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8B73A10E34C; Wed, 19 Feb 2025 18:15:55 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="iIHqKaDJ"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.10]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2EE3210E34C for ; Wed, 19 Feb 2025 18:15:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1739988954; x=1771524954; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=PcLsH4FE/HvPNzaIc6OLyMhygsC1ytB5xVrGsWYsK0Y=; b=iIHqKaDJNOeSs+YPxXtYwPtg++lP0YNn9B+9Khskz8RiM8kdMfqVt5v/ jv7cQ8ngI6yGfL77QGDPWa7yWLh257wZW1qznkZ4kRKaK7Q5RfGxsdaqr RQJ3+osi6CsWIkFRtIlBr2tHmD1COZJGUP51HiTJLOhMFyY1Ln0wg78FC zvn6jCXMya7Kfd/HJBXPwbV+p179SfOfOv5+ZH8imfxC1gK+h4QenwdvJ RGkainGkv41CC2L9oZEa5DAC3yz84U+fVDaqyzbLM3so//cnDSbEtRDUk A0oNEYa7zXnhjmDNSWTND33aO6n2NdMwjZQCNz/HDXdcG6VfU7bCj5O3q w==; X-CSE-ConnectionGUID: 27hXW/7bRbG9yj45IlvOnw== X-CSE-MsgGUID: +i2UMsvoTZC8aJZrDY/Ugg== X-IronPort-AV: E=McAfee;i="6700,10204,11350"; a="58149835" X-IronPort-AV: E=Sophos;i="6.13,299,1732608000"; d="scan'208";a="58149835" Received: from fmviesa006.fm.intel.com ([10.60.135.146]) by orvoesa102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Feb 2025 10:15:54 -0800 X-CSE-ConnectionGUID: Nw3IktROTuGz3Rbg+GYWuA== X-CSE-MsgGUID: AA6+hK7WRZ++0vAA8/tG1w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.13,299,1732608000"; d="scan'208";a="114623790" Received: from orsosgc001.jf.intel.com (HELO orsosgc001.intel.com) ([10.165.21.142]) by fmviesa006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Feb 2025 10:15:53 -0800 Date: Wed, 19 Feb 2025 10:15:52 -0800 Message-ID: <85tt8pol5z.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Harish Chegondi Cc: , , , , Subject: Re: [PATCH v10 4/8] drm/xe/eustall: Add support to read() and poll() EU stall data In-Reply-To: References: 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, 18 Feb 2025 11:53:54 -0800, Harish Chegondi wrote: > > @@ -39,7 +40,9 @@ struct per_xecore_buf { > }; > > struct xe_eu_stall_data_stream { > + bool pollin; > bool enabled; > + wait_queue_head_t poll_wq; > size_t data_record_size; > size_t per_xecore_buf_size; > unsigned int wait_num_reports; > @@ -47,7 +50,11 @@ struct xe_eu_stall_data_stream { > > struct xe_gt *gt; > struct xe_bo *bo; > + /* Lock to protect xecore_buf */ > + struct mutex buf_lock; Why do we need this new lock? I thought we would just be able to use gt->eu_stall->stream_lock? stream_lock is already taken for read(), so we just need to take it for eu_stall_data_buf_poll()? > @@ -357,16 +580,26 @@ static int xe_eu_stall_stream_init(struct xe_eu_stall_data_stream *stream, > max_wait_num_reports); > return -EINVAL; > } > + > + init_waitqueue_head(&stream->poll_wq); > + INIT_DELAYED_WORK(&stream->buf_poll_work, eu_stall_data_buf_poll_work_fn); > + mutex_init(&stream->buf_lock); > + stream->buf_poll_wq = alloc_ordered_workqueue("xe_eu_stall", 0); > + if (!stream->buf_poll_wq) > + return -ENOMEM; > stream->per_xecore_buf_size = per_xecore_buf_size; > stream->sampling_rate_mult = props->sampling_rate_mult; > stream->wait_num_reports = props->wait_num_reports; > stream->data_record_size = xe_eu_stall_data_record_size(gt_to_xe(gt)); > stream->xecore_buf = kcalloc(last_xecore, sizeof(*stream->xecore_buf), GFP_KERNEL); > - if (!stream->xecore_buf) > + if (!stream->xecore_buf) { > + destroy_workqueue(stream->buf_poll_wq); > return -ENOMEM; > + } > > ret = xe_eu_stall_data_buf_alloc(stream, last_xecore); > if (ret) { > + destroy_workqueue(stream->buf_poll_wq); > kfree(stream->xecore_buf); OK, won't block on this, but error unwinding is cleaner with label's and goto's. Also, if stream->buf_lock is needed, mutex_destroy is also needed for error unwinding and also during stream close. > diff --git a/drivers/gpu/drm/xe/xe_trace.h b/drivers/gpu/drm/xe/xe_trace.h > index d5281de04d54..1cc6bfc34ccb 100644 > --- a/drivers/gpu/drm/xe/xe_trace.h > +++ b/drivers/gpu/drm/xe/xe_trace.h > @@ -427,6 +427,39 @@ DEFINE_EVENT(xe_pm_runtime, xe_pm_runtime_get_ioctl, > TP_ARGS(xe, caller) > ); > > +TRACE_EVENT(xe_eu_stall_data_read, > + TP_PROTO(u8 slice, u8 subslice, > + u32 read_ptr, u32 write_ptr, > + u32 read_offset, u32 write_offset, > + size_t total_size), > + TP_ARGS(slice, subslice, read_ptr, write_ptr, > + read_offset, write_offset, total_size), > + > + TP_STRUCT__entry(__field(u8, slice) > + __field(u8, subslice) > + __field(u32, read_ptr) > + __field(u32, write_ptr) > + __field(u32, read_offset) > + __field(u32, write_offset) > + __field(size_t, total_size) > + ), > + > + TP_fast_assign(__entry->slice = slice; > + __entry->subslice = subslice; > + __entry->read_ptr = read_ptr; > + __entry->write_ptr = write_ptr; > + __entry->read_offset = read_offset; > + __entry->write_offset = write_offset; Keep it if we need it, but do we need both the read/write ptr's and offset's? Since offset's are the same as ptr's, but without the ms bit. > + __entry->total_size = total_size; > + ), > + > + TP_printk("slice:%u subslice:%u readptr:0x%x writeptr:0x%x read off:%u write off:%u size:%zu ", > + __entry->slice, __entry->subslice, > + __entry->read_ptr, __entry->write_ptr, > + __entry->read_offset, __entry->write_offset, > + __entry->total_size) > +); > + > #endif > > /* This part must be outside protection */ > -- > 2.48.1 >