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 9A5EACD5BD1 for ; Mon, 1 Jun 2026 23:30:39 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 566B410EA63; Mon, 1 Jun 2026 23:30:39 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="lzioGQib"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.19]) by gabe.freedesktop.org (Postfix) with ESMTPS id ED43410EA63 for ; Mon, 1 Jun 2026 23:30:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1780356638; x=1811892638; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=E0dA/aME8AbiNVGCEExP0dTRt7wv3Yl8UpqcFswIIOw=; b=lzioGQib+oBV/a2ZUgp6hJhaZsI6wbR1bJQb0a/miwUeWhN/5F5PUlTk YMvpAs3JNyXExLIsIKd2jr/vP+lFgBLxQZnyBEffYZLLS2ujMXEunOea0 urk962+AVWZNRR1VeIDaW0QQ4kLUGDmTIe6qF9GmME3KDPsMMYaaHwNR4 iljundzGdNxFjWZ2uWJ/XU7tRsLpY6LTWLsDcZ61A79iRWn1Dl5bMGCFH cbi+InFBk/2gzp+miYoDkp1SsmSfZ8p3rguUjBfrvRzlxH3+A3JYLXbzp 1K7AT91xRiEsogJFzoDb22rZK4vHWsFj/OZzWT2+s/I1KQiihhsMnu+A7 g==; X-CSE-ConnectionGUID: A5CbDI5QSsq2HMksFt9tww== X-CSE-MsgGUID: U2MwfQrxTmaOt5ABT/q33A== X-IronPort-AV: E=McAfee;i="6800,10657,11804"; a="81097917" X-IronPort-AV: E=Sophos;i="6.24,182,1774335600"; d="scan'208";a="81097917" Received: from fmviesa006.fm.intel.com ([10.60.135.146]) by orvoesa111.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Jun 2026 16:30:37 -0700 X-CSE-ConnectionGUID: R5vVhfJXQSSmP8yjl/pH6w== X-CSE-MsgGUID: 2ybEQYbPTQijNClii76+lA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.24,182,1774335600"; d="scan'208";a="239298923" Received: from unknown (HELO adixit-MOBL3.intel.com) ([10.241.243.158]) by fmviesa006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Jun 2026 16:30:37 -0700 Date: Mon, 01 Jun 2026 16:30:36 -0700 Message-ID: <87zf1dltyb.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa Cc: Subject: Re: [PATCH 9/9] drm/xe/rtp: Ensure locking/ref counting for OA whitelists In-Reply-To: References: <20260518234716.1540123-1-ashutosh.dixit@intel.com> <20260518234716.1540123-10-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, 27 May 2026 13:04:31 -0700, Umesh Nerlige Ramappa wrote: > Hi Umesh, > On Mon, May 18, 2026 at 04:47:16PM -0700, Ashutosh Dixit wrote: > > Since multiple OA streams might be open in parallel on a gt, ensure that > > proper locking is in place. Also ensure that OA registers are whitelisted > > when the first OA stream is open and de-whitelisted after the last OA > > stream is closed. > > > > Signed-off-by: Ashutosh Dixit > > --- > > drivers/gpu/drm/xe/xe_oa_types.h | 3 +++ > > drivers/gpu/drm/xe/xe_reg_whitelist.c | 9 +++++++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/drivers/gpu/drm/xe/xe_oa_types.h b/drivers/gpu/drm/xe/xe_oa_types.h > > index 3d9ec8490899c..e876e9be92ba5 100644 > > --- a/drivers/gpu/drm/xe/xe_oa_types.h > > +++ b/drivers/gpu/drm/xe/xe_oa_types.h > > @@ -126,6 +126,9 @@ struct xe_oa_gt { > > > > /** @oa_unit: array of oa_units */ > > struct xe_oa_unit *oa_unit; > > + > > + /** @whitelist_count: number of open streams for which oa registers are whitelisted */ > > + u32 whitelist_count; > > }; > > > > /** > > diff --git a/drivers/gpu/drm/xe/xe_reg_whitelist.c b/drivers/gpu/drm/xe/xe_reg_whitelist.c > > index 50b34c5008df7..1b72f0d2935a2 100644 > > --- a/drivers/gpu/drm/xe/xe_reg_whitelist.c > > +++ b/drivers/gpu/drm/xe/xe_reg_whitelist.c > > @@ -255,6 +255,10 @@ void xe_reg_whitelist_oa_regs(struct xe_gt *gt) > > struct xe_hw_engine *hwe; > > enum xe_hw_engine_id id; > > > > + lockdep_assert_held(>->oa.gt_lock); > > + if (gt->oa.whitelist_count++) > > + return; > > Any reason why not using kref here? Maybe I am wrong, but I looked into it and to me it seems kref is not appropriate to use here. That is because: 1. A kref is first initialized to value 1 using kref_init(). 2. Then subsequently you do kref_get()'s and kref_put()'s 3. Even if we have matching kref_get()'s and kref_put()'s, a final kref_put() is needed to bring the value down to 0 and trigger the release() function Also, there is only a release() function, but nothing to trigger something when the value goes from 0 to 1. So in this case, you'd explicitly need to check the kref value. Further, kref's are really useful for tracking arbitrary/asymmetrical producers/consumers (say as part of dma_fence's), but in a strictly symmetric situation such as what we have here, kref's don't seem to be very useful. (See also previous use of kref in 'struct xe_oa_config' e.g.). So here what we want is, whitelist OA registers when gt->oa.whitelist_count goes from 0 to 1 and dewhitelist them when gt->oa.whitelist_count goes from 1 to 0. Also, we already take 'gt->oa.gt_lock' in both these cases, so even an atomic_t refcount is not needed. So, overall, it seems to me that if we use a kref here, it will look forced and the code will be uglier than what we have here. Let me know what you think about this, but for now I am tending to leaving this as is. Thanks. -- Ashutosh > > + > > for_each_hw_engine(hwe, gt, id) > > __whitelist_oa_regs(hwe, true); > > } > > @@ -270,6 +274,11 @@ void xe_reg_dewhitelist_oa_regs(struct xe_gt *gt) > > struct xe_hw_engine *hwe; > > enum xe_hw_engine_id id; > > > > + lockdep_assert_held(>->oa.gt_lock); > > + xe_assert(gt_to_xe(gt), gt->oa.whitelist_count); > > + if (--gt->oa.whitelist_count) > > + return; > > + > > for_each_hw_engine(hwe, gt, id) > > __whitelist_oa_regs(hwe, false); > > } > > -- > > 2.54.0 > >