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 655CAE7717F for ; Tue, 17 Dec 2024 20:07:51 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3499B10E048; Tue, 17 Dec 2024 20:07:51 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="aKJCklqR"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.13]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4017910E048 for ; Tue, 17 Dec 2024 20:07:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1734466070; x=1766002070; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=1yrqjNOKeKLrrL+1YXGr8oAv2tPFDdfYUe20p4pUIyE=; b=aKJCklqRWa/G8biaZpef3/XenAEexLPmmwSPS4EbfRyol5EgcSDQFVLk wzOHJUYmdEsFg6o+RKjrzEy++H6PWiXJYTk0CyuhpPM0yRsK5i5BEByPT 1n/6NyJ7mu7bb9VE0OjMegpvk7D35pEb5wHAuUBTFRQ6fbRZSa0prDDFA M2gDm4aPHvtHGHA5XZnpwxouktHrAPHWA1pUJgKdkizop/6S2ctGit/Cm 9hUuFexGsYWbMMEldxhW0wZ7wBi00XODjPEdxo5IEiqW05ChZJt4iHnrK u8Bg598SJPyhp/9DoEWHWKZe1h6UDoDnSuXrg71awREd9gpgT8iHFrtf1 g==; X-CSE-ConnectionGUID: zxwV4mrCQ9Sb8bvnoCQWxA== X-CSE-MsgGUID: kwx0JsFgRnq3QMOuhzioxQ== X-IronPort-AV: E=McAfee;i="6700,10204,11289"; a="37748260" X-IronPort-AV: E=Sophos;i="6.12,242,1728975600"; d="scan'208";a="37748260" Received: from orviesa007.jf.intel.com ([10.64.159.147]) by fmvoesa107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Dec 2024 12:07:50 -0800 X-CSE-ConnectionGUID: bG/Pac5WRDy5f3uxY3OXKQ== X-CSE-MsgGUID: VZvgGbsLSZmot03+NC7bfA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.12,224,1728975600"; d="scan'208";a="98081645" Received: from orsosgc001.jf.intel.com (HELO orsosgc001.intel.com) ([10.165.21.142]) by orviesa007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Dec 2024 12:07:50 -0800 Date: Tue, 17 Dec 2024 12:07:49 -0800 Message-ID: <855xni2huy.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Harish Chegondi Cc: , , , , , , , Subject: Re: [PATCH v6 6/7] drm/xe/uapi: Add a device query to get EU stall sampling information In-Reply-To: <6e104a81ccf931e55a577ce816e5821aab57bec4.1734427624.git.harish.chegondi@intel.com> References: <6e104a81ccf931e55a577ce816e5821aab57bec4.1734427624.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, 17 Dec 2024 01:46:56 -0800, Harish Chegondi wrote: > Hi Harish, Only reviewing the uapi, not the implementation. > User space can get the EU stall data record size, EU stall capabilities, > EU stall sampling rates, and per XeCore buffer size with query IOCTL > DRM_IOCTL_XE_DEVICE_QUERY with .query set to DRM_XE_DEVICE_QUERY_EU_STALL. > A struct drm_xe_query_eu_stall will be returned to the user space along > with an array of supported sampling rates sorted in the fastest sampling > rate first order. sampling_rates in struct drm_xe_query_eu_stall will > point to the array of sampling rates. > > Any capabilities in EU stall sampling as of this patch are considered > as base capabilities. Any new capabilities added later will need > a new capabilities flag. s/a new capabilities flag/new capability bits/. Or, better to say something like "New capability bits will be added for any new functionality added later". See OA capability bits in the latest drm-tip. > > v6: Include EU stall sampling rates information and > per XeCore buffer size in the query information. /snip/ > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h > index 4ee3b04a1bb5..40c2d274473e 100644 > --- a/include/uapi/drm/xe_drm.h > +++ b/include/uapi/drm/xe_drm.h > @@ -700,6 +700,7 @@ struct drm_xe_device_query { > #define DRM_XE_DEVICE_QUERY_ENGINE_CYCLES 6 > #define DRM_XE_DEVICE_QUERY_UC_FW_VERSION 7 > #define DRM_XE_DEVICE_QUERY_OA_UNITS 8 > +#define DRM_XE_DEVICE_QUERY_EU_STALL 9 > /** @query: The type of data to query */ > __u32 query; > > @@ -1770,6 +1771,40 @@ enum drm_xe_eu_stall_property_id { > DRM_XE_EU_STALL_PROP_EVENT_REPORT_COUNT, > }; > > +/** > + * struct drm_xe_query_eu_stall - Information about EU stall sampling. > + * > + * If a query is made with a struct @drm_xe_device_query where .query > + * is equal to @DRM_XE_DEVICE_QUERY_EU_STALL, then the reply uses > + * struct @drm_xe_query_eu_stall in .data. > + */ > +struct drm_xe_query_eu_stall { > + /** @extensions: Pointer to the first extension struct, if any */ > + __u64 extensions; > + > + /** @capabilities: EU stall capabilities bit-mask */ > + __u64 capabilities; > +#define DRM_XE_EU_STALL_CAPS_BASE (1 << 0) > + > + /** @record_size: size of each EU stall data record */ > + __u64 record_size; > + > + /** @per_xecore_buf_size: Per XeCore buffer size */ > + __u64 per_xecore_buf_size; This new member has appeared. What is its purpose and how will userspace use it? Basically how is it relevant? > + > + /** @num_sampling_rates: Number of sampling rates supported */ > + __u64 num_sampling_rates; > + > + /** > + * @sampling_rates: Pointer to an array of sampling rates > + * sorted in the fastest sampling rate first order. sorted from fastest to slowest. > + */ > + __u64 *sampling_rates; This should be written as a flexible array as follows: __u64 sampling_rates[]; So sampling rate array is just present at the end of the struct, there should be no separate pointer here in the struct. https://en.wikipedia.org/wiki/Flexible_array_member Also we need to document what units the sampling rates are in. So something like "Sampling rates are specified in number of cycles of the reference clock". So we have decided not to use nanoseconds (so sampling period rather than sampling rate)? Any particular reason for that? Though I am ok either way. > + > + /** @reserved: Reserved */ > + __u64 reserved[5]; Because flexible arrays must be the last field in a structure, this reserved field should be moved before num_sampling_rates field. > +}; > + > #if defined(__cplusplus) > } > #endif > -- > 2.47.0 > Ashutosh