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 3AE55E936FF for ; Thu, 5 Oct 2023 05:27:21 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E43B710E1B8; Thu, 5 Oct 2023 05:27:20 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.136]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0C9C910E1B8 for ; Thu, 5 Oct 2023 05:27:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1696483639; x=1728019639; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=TPQ9MSDAOXURGptTh+t4OjqLj+X5XBD04NqeRidyFm8=; b=Nqjlm24nhlwRh/1GSlpjmsMSYhZDcyTdwC3X8jyXQSPkXA1HsFhusuZW pI+npl1tMYjYwxmb6IJ/SmpA2eCl5vmAyCp2Z7u8Q2tkOAy2JqrtwN7WF KbOnslW5GyJpgGekE+qAxvZpA4PJaQtTUdjh1XLF92/ECeS3ph+xVkHi2 3hW9bmCZQN67/NuTUzjTl4iKTbLrxtHU5bX6TcOBd/2UDIaiTZzu9oFr/ h7lAxhIVuBqaYIw0x1ZZphNM6UGPkVtWIduide68Febu4Lzq8jatimZGi aXSc9XWLMJ8P+4eUyGUvSiZAHQ4zuH+Z3fh2xcI+aMfPmKqCiWCVcaPHk g==; X-IronPort-AV: E=McAfee;i="6600,9927,10853"; a="362764445" X-IronPort-AV: E=Sophos;i="6.03,202,1694761200"; d="scan'208";a="362764445" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Oct 2023 22:27:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10853"; a="701514600" X-IronPort-AV: E=Sophos;i="6.03,202,1694761200"; d="scan'208";a="701514600" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.101.181]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Oct 2023 22:27:15 -0700 Date: Wed, 04 Oct 2023 22:27:14 -0700 Message-ID: <87o7hd1vwt.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa In-Reply-To: References: <20230919161049.2307855-1-ashutosh.dixit@intel.com> <20230919161049.2307855-14-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/29.1 (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 Subject: Re: [Intel-xe] [PATCH 13/21] drm/xe/uapi: Multiplex PERF ops through a single PERF ioctl 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: , Cc: intel-xe@lists.freedesktop.org Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Tue, 03 Oct 2023 19:23:24 -0700, Umesh Nerlige Ramappa wrote: > Hi Umesh, > On Tue, Sep 19, 2023 at 09:10:41AM -0700, Ashutosh Dixit wrote: > > Since we are already mulitplexing multiple perf counter stream types > > through the PERF layer, it seems odd to retain separate ioctls for perf > > op's (add/remove config). In fact it seems logical to also multiplex these > > ops through a single PERF ioctl. This also affords greater flexibility to > > add stream specific ops if needed for different perf stream types. > > > > Signed-off-by: Ashutosh Dixit > > --- > > drivers/gpu/drm/xe/xe_device.c | 5 +---- > > drivers/gpu/drm/xe/xe_perf.c | 32 ++++++++------------------------ > > drivers/gpu/drm/xe/xe_perf.h | 4 +--- > > include/uapi/drm/xe_drm.h | 16 ++++++++++------ > > 4 files changed, 20 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c > > index 770b9fe6e65df..24018a0801788 100644 > > --- a/drivers/gpu/drm/xe/xe_device.c > > +++ b/drivers/gpu/drm/xe/xe_device.c > > @@ -115,10 +115,7 @@ static const struct drm_ioctl_desc xe_ioctls[] = { > > DRM_RENDER_ALLOW), > > DRM_IOCTL_DEF_DRV(XE_VM_MADVISE, xe_vm_madvise_ioctl, DRM_RENDER_ALLOW), > > > > - DRM_IOCTL_DEF_DRV(XE_PERF_OPEN, xe_perf_open_ioctl, DRM_RENDER_ALLOW), > > - DRM_IOCTL_DEF_DRV(XE_PERF_ADD_CONFIG, xe_perf_add_config_ioctl, DRM_RENDER_ALLOW), > > - DRM_IOCTL_DEF_DRV(XE_PERF_REMOVE_CONFIG, xe_perf_remove_config_ioctl, DRM_RENDER_ALLOW), > > - > > + DRM_IOCTL_DEF_DRV(XE_PERF, xe_perf_ioctl, DRM_RENDER_ALLOW), > > }; > > > > static const struct file_operations xe_driver_fops = { > > diff --git a/drivers/gpu/drm/xe/xe_perf.c b/drivers/gpu/drm/xe/xe_perf.c > > index 0f747af59f245..f8d7eae8fffe0 100644 > > --- a/drivers/gpu/drm/xe/xe_perf.c > > +++ b/drivers/gpu/drm/xe/xe_perf.c > > @@ -6,37 +6,21 @@ > > #include "xe_oa.h" > > #include "xe_perf.h" > > > > -int xe_perf_open_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > > +int xe_oa_ioctl(struct drm_device *dev, struct drm_xe_perf_param *arg, struct drm_file *file) > > { > > - struct drm_xe_perf_param *arg = data; > > - > > - if (arg->extensions) > > - return -EINVAL; > > - > > - switch (arg->perf_type) { > > - case XE_PERF_TYPE_OA: > > + switch (arg->perf_op) { > > + case XE_PERF_STREAM_OPEN: > > return xe_oa_stream_open_ioctl(dev, (void *)arg->param, file); > > It's a nice idea to reduce the ioctls, but if your struct drm_xe_perf_param > *arg is overloaded based on the PERF_OP passed, then I would recommend > validating that the right arg is passed for the corresponding OP. I am not following what you mean here: which right arg for which OP? The PERF layer only demultiplexes based on perf_type (say OA/XYZ etc.). The perf_op belongs to the perf_type layer (say OA), not the PERF layer. It is the job of the perf_type layer (OA) to validate the perf_op, not the job of the PERF layer. It is just convenient to include the perf_op as part of 'struct drm_xe_perf_param' (rather than inventing yet another layer there). See the function xe_perf_ioctl() in the patch. The xe_oa_ioctl function above could possibly be moved into xe_oa.c. I just left it in xe_perf.c since it didn't seem to matter much. But I am open to doing that. > Ideally I wouldn't go that route since that would require some sort of > signature in the arg which would identify it as the correct > param. Instead I would be okay with retaining separate ioctls for the 3 > operations. If we were not doing this multiplexing based on perf_type (as in i915) we could have separate ioctl's for each operation. But since here we have anyway introduced a multiplxing layer, to me it makes no sense to have separate operation ioctl's (only disadvantags and no advantages). (Note that the multiplexing layer implies a (non-obvious) additional copy_from_user per operation visible in the previous "drm/xe/uapi: "Perf" layer to support multiple perf counter stream types" patch). Also we cannot assume that a future stream type will only have 3 operations as i915 OA did. The OPEN/ADD_CONFIG/CLOSE are really OA specific operations. But it appears other potential perf_type's will also be able to use them, at least initially that is why they are left defined as PERF_OP's (rather than OA_OP's) in xe_drm.h. New stream types are free to introduce new ops in this design. So retaining the ops inside a single PERF ioctl eliminates the need for introducing a new ioctl each time a stream type introduces a new OP. Thanks. -- Ashutosh