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 212F2C02182 for ; Thu, 23 Jan 2025 18:52:38 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id AFB9A10E0CF; Thu, 23 Jan 2025 18:52:37 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="HEX0yWrc"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.17]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4A9DA10E02A for ; Thu, 23 Jan 2025 18:51: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=1737658306; x=1769194306; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=yggW6RnXQfVrshTt2nR0knp94Crv34UDBX8gEDaBZSI=; b=HEX0yWrc6RuJbQNCcoOFckoskLHR5gtBr8I0vusdCH1CWwfCw0lzCRMc broutS1uXNgZLysC2w+wKXPJdxMbAVpTuwJ/7nEraxGIduNEsjOh3gJmi 00/VHhdk+EuFwS3IO1vaAdXnbdOLiSCFCgw26GHgfvQaQ6zDYgMpU6ZV1 nfuOVVzYqvrWF51z8f7R/zx/qFGsKQxvI6/9ClTlws30M3ZdiliTHRR0H puMVbHRiZZfX9neCcGNbU6aGD9ThQHI0Upp0Nx1/UdpUthXaoG0tx9Sc7 0FRUn8AJ7o/Ok3XX44jYTUK6bfn6dy2sKXIqRaXTyKeMz9XqtT/hFZg/w Q==; X-CSE-ConnectionGUID: a9sqLZDoRTmVWdRGFbZO/A== X-CSE-MsgGUID: CmBRmE61QuyvZ1Wws3T1tA== X-IronPort-AV: E=McAfee;i="6700,10204,11324"; a="38208028" X-IronPort-AV: E=Sophos;i="6.13,229,1732608000"; d="scan'208";a="38208028" Received: from fmviesa007.fm.intel.com ([10.60.135.147]) by orvoesa109.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Jan 2025 10:51:45 -0800 X-CSE-ConnectionGUID: R74RH6ZAQN6lwyLm5Fxp0Q== X-CSE-MsgGUID: e+pVFrpBTuS/iNAQAaITRA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.13,229,1732608000"; d="scan'208";a="107455191" Received: from orsosgc001.jf.intel.com (HELO orsosgc001.intel.com) ([10.165.21.142]) by fmviesa007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Jan 2025 10:51:44 -0800 Date: Thu, 23 Jan 2025 10:51:43 -0800 Message-ID: <85ed0tfjnk.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Harish Chegondi Cc: , , , , , , , , Subject: Re: [PATCH v8 3/7] drm/xe/eustall: Implement EU stall sampling APIs for Xe_HPC In-Reply-To: <28e9b33c1fd5164d611d1ea3caa45388278efe43.1736970203.git.harish.chegondi@intel.com> References: <28e9b33c1fd5164d611d1ea3caa45388278efe43.1736970203.git.harish.chegondi@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 12:02:09 -0800, Harish Chegondi wrote: > Hi Harish, Still reviewing but here's the next set of review comments. Since this is a huge 800 line patch, I'll just keep sending comments in email about section of the code I am able to review, so that you can continue to work on addressing the review comments in parallel. I will also drop most people on the Cc: from these emails in the future. Let them read the email on the intel-xe mailing list if interested. > +static u32 > +gen_eustall_base(struct xe_eu_stall_data_stream *stream, bool enable) > +{ > + u32 val = xe_bo_ggtt_addr(stream->bo); > + u32 sz; > + > + XE_WARN_ON(!IS_ALIGNED(val, 64)); > + > + switch (stream->per_xecore_buf_size) { > + case SZ_128K: > + sz = 0; > + break; > + case SZ_256K: > + sz = 1; > + break; > + case SZ_512K: > + sz = 2; > + break; Hmm this switch statement is not needed. This is just: sz = stream->per_xecore_buf_size / SZ_256K; > + default: > + xe_gt_warn(stream->gt, "Missing case per XeCore buffer size == %lu)\n", > + (long)(stream->per_xecore_buf_size)); > + sz = 2; If you need this check, can we use BUILD_BUG_ON(per_xecore_buf_size ...) here? Or skip the check, this is an internal variable, we don't get this from userspace, correct? > +/** > + * xe_eu_stall_stream_open_locked - Open a EU stall data stream FD. > + * @dev: drm device instance > + * @props: individually validated u64 property value pairs > + * @file: drm file > + * > + * Returns: zero on success or a negative error code. > + */ > +static int > +xe_eu_stall_stream_open_locked(struct drm_device *dev, > + struct eu_stall_open_properties *props, > + struct drm_file *file) > +{ > + struct xe_device *xe = to_xe_device(dev); > + struct xe_eu_stall_data_stream *stream; > + struct xe_gt *gt = props->gt; > + unsigned long f_flags = 0; > + int ret, stream_fd; > + > + if (!has_eu_stall_sampling_support(xe)) { Wondering if this should be a bit in xe_graphics_desc? Anyway we can live with this for now. Also why is this check here in locked()? It should be the first check in xe_eu_stall_stream_open? > + xe_gt_dbg(gt, "EU stall monitoring is not supported on this platform\n"); > + return -EPERM; > + } > + > + if (xe_observation_paranoid && !perfmon_capable()) { This check should also be xe_eu_stall_stream_open. Please reorder the checks and distribute them correctly between open and open _locked. locked() should only contain checks where we need to hold the lock. See xe_oa.c for similar checks there. I will review the error rewinding etc in the next rev since it will change with the reordering. > @@ -252,10 +964,15 @@ int xe_eu_stall_stream_open(struct drm_device *dev, > { > struct xe_device *xe = to_xe_device(dev); > struct eu_stall_open_properties props; > - int ret, stream_fd; > + int ret; > > memset(&props, 0, sizeof(struct eu_stall_open_properties)); > > + /* Set default values */ > + props.gt = NULL; Don't need? Already initialized to 0 above. > + props.eu_stall_sampling_rate = 4; Why not call this sampling_rate_mult or something like that? eu_stall_ is an unnecessary repetition, everything here is eu_stall! > + props.wait_num_reports = 1; > + > ret = xe_eu_stall_user_extensions(xe, data, &props); > if (ret) > return ret; > @@ -265,19 +982,9 @@ int xe_eu_stall_stream_open(struct drm_device *dev, > return -EINVAL; > } > > - if (xe_observation_paranoid && !perfmon_capable()) { > - xe_gt_dbg(props.gt, "Insufficient privileges for EU stall monitoring\n"); > - return -EACCES; > - } > + mutex_lock(&props.gt->eu_stall->lock); > + ret = xe_eu_stall_stream_open_locked(dev, &props, file); > + mutex_unlock(&props.gt->eu_stall->lock); > > - if (!has_eu_stall_sampling_support(xe)) { > - xe_gt_dbg(props.gt, "EU stall monitoring is not supported on this platform\n"); > - return -EPERM; > - } > - stream_fd = anon_inode_getfd("[xe_eu_stall]", &fops_eu_stall, > - NULL, 0); > - if (stream_fd < 0) > - xe_gt_dbg(props.gt, "EU stall inode get fd failed : %d\n", stream_fd); > - > - return stream_fd; > + return ret; > } > diff --git a/drivers/gpu/drm/xe/xe_eu_stall.h b/drivers/gpu/drm/xe/xe_eu_stall.h > index 3447958a7a22..f97c8bf8e852 100644 > --- a/drivers/gpu/drm/xe/xe_eu_stall.h > +++ b/drivers/gpu/drm/xe/xe_eu_stall.h > @@ -6,6 +6,49 @@ > #ifndef __XE_EU_STALL_H__ > #define __XE_EU_STALL_H__ > > +#include "xe_gt_types.h" > + > +struct per_xecore_buf { > + u8 *vaddr; > + u32 write; > + u32 read; > + /* lock to protect read and write pointers */ > + struct mutex lock; Let us call this buf_lock or ptr_lock or something like that. I don't like generic names like 'lock' because it makes it really hard to search the code for the locks when they are just named 'lock'. > +}; > + > +/** > + * struct xe_eu_stall_data_stream - state of EU stall data stream FD > + */ > +struct xe_eu_stall_data_stream { > + bool pollin; > + bool enabled; > + u64 poll_period; > + u32 wait_num_reports; > + size_t per_xecore_buf_size; > + wait_queue_head_t poll_wq; > + > + struct xe_gt *gt; > + struct xe_bo *bo; > + struct per_xecore_buf *xecore_buf; > + struct { > + xe_dss_mask_t mask; > + } data_drop; > + struct hrtimer poll_check_timer; > + struct work_struct buf_check_work; > + struct workqueue_struct *buf_check_wq; > +}; > + > +struct xe_eu_stall_gt { > + /* Lock to protect stream */ > + struct mutex lock; Similarly here call this stream_lock or something like that. Thanks. -- Ashutosh