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 8B444C021AA for ; Wed, 19 Feb 2025 15:27:34 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 41C6D10E839; Wed, 19 Feb 2025 15:27:34 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="Qtp8arYr"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.15]) by gabe.freedesktop.org (Postfix) with ESMTPS id 72FBB10E103 for ; Wed, 19 Feb 2025 15:27:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1739978852; x=1771514852; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=g/AwybZ2d8FobG+i4mCkzU9J2qS4hUGeq2qgNQp52wI=; b=Qtp8arYroc5ju1nGjjngUXjQpgMVr85MSi2Aql7+HO7FFCkAfsTXlErm ewDJsdn7dNxPuqFez2N3OWYyEa5CeZC9n18VbZhDx/dJM1eTnv/fcQ3A+ U3F6853NDekloKfNAC/sQ2YeuB7NfNDYZ/sOUhvkCetjw7z8pNfWxGh+s +N183nBZ0FD+1TCxJXWh1rlsbSbWHsZihVndkbimySuH1ZYcLlV57c0Xg 98XetfTNuVSAErernSAO0GO97kG8AOKVONOCfIVyOfuqXa9KuTebqR0Li abR/joWkitf2tav1xBC4sNccGWap2dFAOgjlVaqX9mUJ3nBDwanMbgfNy A==; X-CSE-ConnectionGUID: g1xA5AVwTkWV4I/sgYxZKA== X-CSE-MsgGUID: /Lw1J4qcR6u7NAnLd4JuVw== X-IronPort-AV: E=McAfee;i="6700,10204,11350"; a="44368932" X-IronPort-AV: E=Sophos;i="6.13,299,1732608000"; d="scan'208";a="44368932" Received: from orviesa004.jf.intel.com ([10.64.159.144]) by orvoesa107.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Feb 2025 07:27:32 -0800 X-CSE-ConnectionGUID: buTyeXyySsimsOodiMO/Gw== X-CSE-MsgGUID: nS6W9JDRReyBDKSzRpigsg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.13,299,1732608000"; d="scan'208";a="119720085" Received: from orsosgc001.jf.intel.com (HELO orsosgc001.intel.com) ([10.165.21.142]) by orviesa004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Feb 2025 07:27:30 -0800 Date: Wed, 19 Feb 2025 07:27:24 -0800 Message-ID: <85v7t6neeb.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Harish Chegondi Cc: Subject: Re: [PATCH v10 3/8] drm/xe/eustall: Add support to init, enable and disable EU stall sampling In-Reply-To: <1448af14b3edfb3673eb6992741b7176c9edc1ee.1739906138.git.harish.chegondi@intel.com> References: <1448af14b3edfb3673eb6992741b7176c9edc1ee.1739906138.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 Tue, 18 Feb 2025 11:53:53 -0800, Harish Chegondi wrote: > > +static void xe_eu_stall_stream_free(struct xe_eu_stall_data_stream *stream) > +{ > + struct xe_gt *gt = stream->gt; > + > + gt->eu_stall->stream = NULL; > + kfree(stream); > +} > + > +static void xe_eu_stall_data_buf_destroy(struct xe_eu_stall_data_stream *stream) > +{ > + xe_bo_unpin_map_no_vm(stream->bo); > + kfree(stream->xecore_buf); > +} > + > +static int xe_eu_stall_data_buf_alloc(struct xe_eu_stall_data_stream *stream, > + u16 last_xecore) > +{ > + struct xe_tile *tile = stream->gt->tile; > + struct xe_bo *bo; > + u32 size; > + > + size = stream->per_xecore_buf_size * last_xecore; > + > + bo = xe_bo_create_pin_map_at_aligned(tile->xe, tile, NULL, > + size, ~0ull, ttm_bo_type_kernel, > + XE_BO_FLAG_SYSTEM | XE_BO_FLAG_GGTT, SZ_64); > + if (IS_ERR(bo)) > + return PTR_ERR(bo); > + > + XE_WARN_ON(!IS_ALIGNED(xe_bo_ggtt_addr(bo), SZ_64)); > + stream->bo = bo; > + > + return 0; > +} /snip/ > +static int xe_eu_stall_stream_init(struct xe_eu_stall_data_stream *stream, > + struct eu_stall_open_properties *props) > +{ > + unsigned int max_wait_num_reports, xecore, last_xecore, num_xecores; > + struct per_xecore_buf *xecore_buf; > + struct xe_gt *gt = stream->gt; > + xe_dss_mask_t all_xecores; > + u16 group, instance; > + u32 vaddr_offset; > + int ret; > + > + bitmap_or(all_xecores, gt->fuse_topo.g_dss_mask, gt->fuse_topo.c_dss_mask, > + XE_MAX_DSS_FUSE_BITS); > + num_xecores = bitmap_weight(all_xecores, XE_MAX_DSS_FUSE_BITS); > + last_xecore = xe_gt_topology_mask_last_dss(all_xecores) + 1; > + > + max_wait_num_reports = num_data_rows(per_xecore_buf_size * num_xecores); > + if (props->wait_num_reports == 0 || props->wait_num_reports > max_wait_num_reports) { > + xe_gt_dbg(gt, "Invalid EU stall event report count %u\n", > + props->wait_num_reports); > + xe_gt_dbg(gt, "Minimum event report count is 1, maximum is %u\n", > + max_wait_num_reports); > + return -EINVAL; > + } > + 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) > + return -ENOMEM; How about moving this stream->xecore_buf allocation stuff into xe_eu_stall_data_buf_alloc? Just move it before stream->bo allocation in xe_eu_stall_data_buf_alloc. Because in xe_eu_stall_data_buf_destroy() we are freeing both stream->bo and stream->xecore_buf. So if we move this stream->xecore_buf allocation into xe_eu_stall_data_buf_alloc, xe_eu_stall_data_buf_alloc and xe_eu_stall_data_buf_destroy will be completely symmetric. And by "data_buf" we can understand stream->bo and stream->xecore_buf taken together. There are other ways of doing this too but I think this is the simplest change. After addressing the above comment above, the rest lgtm, so this patch is also: Reviewed-by: Ashutosh Dixit