Message ID | 20230427175340.1280952-9-robdclark@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp439580vqo; Thu, 27 Apr 2023 11:00:06 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6XtcYDUM+q+1daR74wOZpDYG7Sp1HT5C5Cl/VG44xEYwXVXFDdVTJIb+VSYo4QUg+iuVbc X-Received: by 2002:a17:902:dacb:b0:1a3:dcc1:307d with SMTP id q11-20020a170902dacb00b001a3dcc1307dmr8192040plx.23.1682618406362; Thu, 27 Apr 2023 11:00:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1682618406; cv=none; d=google.com; s=arc-20160816; b=Z8ZZkPhejw/nwL9KSj2QS0N5///XXX/a2tghBmX9SJ7RbUNQwXoSxxXQcM+nBvioW0 VN8tWjami+mkOucA+G4oSFntAF+0yTfZ1jq3i97mKrBqcxN6fleVmhTMBnsBCN1xYepz DCBdWQbwWF67ZZX3ccNMG68JWM+al8uJJt6sFcKAi+48p1Qo2aAduzkWWOuFCRGr70qI 0Kp20Sdc/Jr6ojK/kvqiqlTi34lTB0Cs47BRr9ScLgF/k6ztcwDmT09N4oGlsXwn7qKn k+jquuz3vsZ4cfrWMxMgbaFnhZQsl8oi4kfWcXiqaCBND66rSwYjKqaG+jUk91pgKxZ0 ZTaw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=sGLJmibW87WAJcS5t0uIh72jCVPBIb7Gj0WXUx1P9zU=; b=pFDhxrEZo1mYhdjhtZVcO0XjDTKyWYUvnHT/mNPrk+4SUBscBZtCsvJYhmMSZlBJ4Q BMj7BiSZ29MnOVezduhKBIHDJkaFG4NCPkXQS7a7Kyv73uqMkTbz0NGGRwUeQBj3j5W+ n79NYbmaG0tWk4vKV+pJPrKbHne7j9R/utId55FOFThZ/6OVWDOG2fUcYPTHwlMvbEbi /rmsTY85FSd5ue4R41RHwLx7Rvze6hQNlS5M12Y6DaU5wSNVZHyW8OWmV58knlbtHApu OVKYJd4mTwL1Ww8JcIl+mPdq9DtBGypIyqLs4XguoDPUd/v+qT4NmXDFlFYwLYxlCv3M bTlA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=QMXQRPId; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id q13-20020a170902bd8d00b001a531f29044si18141471pls.521.2023.04.27.10.59.53; Thu, 27 Apr 2023 11:00:06 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=QMXQRPId; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S244283AbjD0Ry7 (ORCPT <rfc822;zxc52fgh@gmail.com> + 99 others); Thu, 27 Apr 2023 13:54:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34394 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244535AbjD0RyZ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 27 Apr 2023 13:54:25 -0400 Received: from mail-pf1-x42d.google.com (mail-pf1-x42d.google.com [IPv6:2607:f8b0:4864:20::42d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5A41A3C15; Thu, 27 Apr 2023 10:54:11 -0700 (PDT) Received: by mail-pf1-x42d.google.com with SMTP id d2e1a72fcca58-64115eef620so6797419b3a.1; Thu, 27 Apr 2023 10:54:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1682618051; x=1685210051; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=sGLJmibW87WAJcS5t0uIh72jCVPBIb7Gj0WXUx1P9zU=; b=QMXQRPIdXA5q5Km0eEBU7rKyuFO/cgMrtoDCMJdyNqAiJII2NpWeJLh5tHe70Cy232 Sm5bFKWRn1oSSzIdYYglEqg8J6RUhcSkcWnMH7d4qgcvGCmFinBCuh81eI4k4yWHVpvx ZGSfIBbB+9WYYIJU6QL3JOedPPhjgrhrXsvSGBPHyj1zrMv0q578RG/AT6Ue8xSs2fL3 VqFTs5aUbk2lfTToiCebxdTsxyY8QfQ8af5msEFx2/6VxM7sz1Mj6jn3ZRQ6NcpfhnBz smFVLkXfhebsJulsoQqEEQZQriQbsmux38e2ELM+/aEBKnk7Y2f+FUt5q8Q7P6pfwUeA 1fdw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682618051; x=1685210051; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=sGLJmibW87WAJcS5t0uIh72jCVPBIb7Gj0WXUx1P9zU=; b=cBhd0hWS4Ys++9yltsjwGNJMNB03d1QShIL5r+kwcpuVyECHY7/Jl/WWkcWoXnejoK djev7l/IunKyQK6Dy0Es9ZWI9Qc/POP0nTbA5gdO9WYcZqPK+I4ZcrETDGq/zmoonqon l//UNBv1M+LfLXVoW3ubh26ck0ENhAzyEgvbLqkzSbQWHAKGoCBSd5yfnlKWXWgIUQMD 0k1OQ0a1H3eYyRmVaLnnhmZj9NZdP05xgs5kyK3ZFnSPF17q3NokT78QLtFwGckBxnaD tn9bgYfGRIcTCawR6G57s+R6unKo02+PX2LrQyPNB97froZ3kL3yBfAssbtjRpGMtOrS LykQ== X-Gm-Message-State: AC+VfDxs1g5B3LSbxALWkO57THUhN9NKfsbsCw5uEL5uRmH+QfgKWD7E cPl7/4wtIRrII8CHfTxqhFA= X-Received: by 2002:a05:6a20:918f:b0:f0:3fc4:744f with SMTP id v15-20020a056a20918f00b000f03fc4744fmr3333199pzd.8.1682618050907; Thu, 27 Apr 2023 10:54:10 -0700 (PDT) Received: from localhost ([2a00:79e1:abd:4a00:61b:48ed:72ab:435b]) by smtp.gmail.com with ESMTPSA id y189-20020a62cec6000000b00640defda6d2sm6860749pfg.207.2023.04.27.10.54.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Apr 2023 10:54:10 -0700 (PDT) From: Rob Clark <robdclark@gmail.com> To: dri-devel@lists.freedesktop.org Cc: freedreno@lists.freedesktop.org, Daniel Vetter <daniel@ffwll.ch>, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>, Boris Brezillon <boris.brezillon@collabora.com>, Christopher Healy <healych@amazon.com>, Emil Velikov <emil.l.velikov@gmail.com>, =?utf-8?q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>, Rob Clark <robdclark@chromium.org>, David Airlie <airlied@gmail.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, Jonathan Corbet <corbet@lwn.net>, linux-doc@vger.kernel.org (open list:DOCUMENTATION), linux-kernel@vger.kernel.org (open list) Subject: [PATCH v2 8/9] drm/fdinfo: Add comm/cmdline override fields Date: Thu, 27 Apr 2023 10:53:32 -0700 Message-Id: <20230427175340.1280952-9-robdclark@gmail.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230427175340.1280952-1-robdclark@gmail.com> References: <20230427175340.1280952-1-robdclark@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1764353277721059960?= X-GMAIL-MSGID: =?utf-8?q?1764353277721059960?= |
Series |
drm: fdinfo memory stats
|
|
Commit Message
Rob Clark
April 27, 2023, 5:53 p.m. UTC
From: Rob Clark <robdclark@chromium.org> These are useful in particular for VM scenarios where the process which has opened to drm device file is just a proxy for the real user in a VM guest. Signed-off-by: Rob Clark <robdclark@chromium.org> --- Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++ drivers/gpu/drm/drm_file.c | 15 +++++++++++++++ include/drm/drm_file.h | 19 +++++++++++++++++++ 3 files changed, 52 insertions(+)
Comments
On 27/04/2023 18:53, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > These are useful in particular for VM scenarios where the process which > has opened to drm device file is just a proxy for the real user in a VM > guest. > > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++ > drivers/gpu/drm/drm_file.c | 15 +++++++++++++++ > include/drm/drm_file.h | 19 +++++++++++++++++++ > 3 files changed, 52 insertions(+) > > diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst > index 58dc0d3f8c58..e4877cf8089c 100644 > --- a/Documentation/gpu/drm-usage-stats.rst > +++ b/Documentation/gpu/drm-usage-stats.rst > @@ -73,6 +73,24 @@ scope of each device, in which case `drm-pdev` shall be present as well. > Userspace should make sure to not double account any usage statistics by using > the above described criteria in order to associate data to individual clients. > > +- drm-comm-override: <valstr> > + > +Returns the client executable override string. Some drivers support letting > +userspace override this in cases where the userspace is simply a "proxy". > +Such as is the case with virglrenderer drm native context, where the host > +process is just forwarding command submission, etc, from guest userspace. > +This allows the proxy to make visible the executable name of the actual > +app in the VM guest. > + > +- drm-cmdline-override: <valstr> > + > +Returns the client cmdline override string. Some drivers support letting > +userspace override this in cases where the userspace is simply a "proxy". > +Such as is the case with virglrenderer drm native context, where the host > +process is just forwarding command submission, etc, from guest userspace. > +This allows the proxy to make visible the cmdline of the actual app in the > +VM guest. Perhaps it would be okay to save space here by not repeating the description, like: drm-comm-override: <valstr> drm-cmdline-override: <valstr> Long description blah blah... This allows the proxy to make visible the _executable name *and* command line_ blah blah.. > + > Utilization > ^^^^^^^^^^^ > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > index 9321eb0bf020..d7514c313af1 100644 > --- a/drivers/gpu/drm/drm_file.c > +++ b/drivers/gpu/drm/drm_file.c > @@ -178,6 +178,8 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor) > spin_lock_init(&file->master_lookup_lock); > mutex_init(&file->event_read_lock); > > + mutex_init(&file->override_lock); > + > if (drm_core_check_feature(dev, DRIVER_GEM)) > drm_gem_open(dev, file); > > @@ -292,6 +294,8 @@ void drm_file_free(struct drm_file *file) > WARN_ON(!list_empty(&file->event_list)); > > put_pid(file->pid); > + kfree(file->override_comm); > + kfree(file->override_cmdline); > kfree(file); > } > > @@ -995,6 +999,17 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f) > PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); > } > > + mutex_lock(&file->override_lock); You could add a fast unlocked check before taking the mutex for no risk apart a transient false negative. For 99.9999% of userspace it would mean no pointless lock/unlock cycle. > + if (file->override_comm) { > + drm_printf(&p, "drm-comm-override:\t%s\n", > + file->override_comm); > + } > + if (file->override_cmdline) { > + drm_printf(&p, "drm-cmdline-override:\t%s\n", > + file->override_cmdline); > + } > + mutex_unlock(&file->override_lock); > + > if (dev->driver->show_fdinfo) > dev->driver->show_fdinfo(&p, file); > } > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index 1339e925af52..604d05fa6f0c 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -370,6 +370,25 @@ struct drm_file { > */ > struct drm_prime_file_private prime; > > + /** > + * @comm: Overridden task comm > + * > + * Accessed under override_lock > + */ > + char *override_comm; > + > + /** > + * @cmdline: Overridden task cmdline > + * > + * Accessed under override_lock > + */ > + char *override_cmdline; > + > + /** > + * @override_lock: Serialize access to override_comm and override_cmdline > + */ > + struct mutex override_lock; > + I don't think this should go to drm just yet though. Only one driver can make use of it so I'd leave it for later and print from msm_show_fdinfo for now. Regards, Tvrtko > /* private: */ > #if IS_ENABLED(CONFIG_DRM_LEGACY) > unsigned long lock_count; /* DRI1 legacy lock count */
On Fri, Apr 28, 2023 at 4:05 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 27/04/2023 18:53, Rob Clark wrote: > > From: Rob Clark <robdclark@chromium.org> > > > > These are useful in particular for VM scenarios where the process which > > has opened to drm device file is just a proxy for the real user in a VM > > guest. > > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++ > > drivers/gpu/drm/drm_file.c | 15 +++++++++++++++ > > include/drm/drm_file.h | 19 +++++++++++++++++++ > > 3 files changed, 52 insertions(+) > > > > diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst > > index 58dc0d3f8c58..e4877cf8089c 100644 > > --- a/Documentation/gpu/drm-usage-stats.rst > > +++ b/Documentation/gpu/drm-usage-stats.rst > > @@ -73,6 +73,24 @@ scope of each device, in which case `drm-pdev` shall be present as well. > > Userspace should make sure to not double account any usage statistics by using > > the above described criteria in order to associate data to individual clients. > > > > +- drm-comm-override: <valstr> > > + > > +Returns the client executable override string. Some drivers support letting > > +userspace override this in cases where the userspace is simply a "proxy". > > +Such as is the case with virglrenderer drm native context, where the host > > +process is just forwarding command submission, etc, from guest userspace. > > +This allows the proxy to make visible the executable name of the actual > > +app in the VM guest. > > + > > +- drm-cmdline-override: <valstr> > > + > > +Returns the client cmdline override string. Some drivers support letting > > +userspace override this in cases where the userspace is simply a "proxy". > > +Such as is the case with virglrenderer drm native context, where the host > > +process is just forwarding command submission, etc, from guest userspace. > > +This allows the proxy to make visible the cmdline of the actual app in the > > +VM guest. > > Perhaps it would be okay to save space here by not repeating the > description, like: > > drm-comm-override: <valstr> > drm-cmdline-override: <valstr> > > Long description blah blah... > This allows the proxy to make visible the _executable name *and* command > line_ blah blah.. > > > + > > Utilization > > ^^^^^^^^^^^ > > > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > > index 9321eb0bf020..d7514c313af1 100644 > > --- a/drivers/gpu/drm/drm_file.c > > +++ b/drivers/gpu/drm/drm_file.c > > @@ -178,6 +178,8 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor) > > spin_lock_init(&file->master_lookup_lock); > > mutex_init(&file->event_read_lock); > > > > + mutex_init(&file->override_lock); > > + > > if (drm_core_check_feature(dev, DRIVER_GEM)) > > drm_gem_open(dev, file); > > > > @@ -292,6 +294,8 @@ void drm_file_free(struct drm_file *file) > > WARN_ON(!list_empty(&file->event_list)); > > > > put_pid(file->pid); > > + kfree(file->override_comm); > > + kfree(file->override_cmdline); > > kfree(file); > > } > > > > @@ -995,6 +999,17 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f) > > PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); > > } > > > > + mutex_lock(&file->override_lock); > > You could add a fast unlocked check before taking the mutex for no risk > apart a transient false negative. For 99.9999% of userspace it would > mean no pointless lock/unlock cycle. I'm not sure I get your point? This needs to be serialized against userspace setting the override values > > > + if (file->override_comm) { > > + drm_printf(&p, "drm-comm-override:\t%s\n", > > + file->override_comm); > > + } > > + if (file->override_cmdline) { > > + drm_printf(&p, "drm-cmdline-override:\t%s\n", > > + file->override_cmdline); > > + } > > + mutex_unlock(&file->override_lock); > > + > > if (dev->driver->show_fdinfo) > > dev->driver->show_fdinfo(&p, file); > > } > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > > index 1339e925af52..604d05fa6f0c 100644 > > --- a/include/drm/drm_file.h > > +++ b/include/drm/drm_file.h > > @@ -370,6 +370,25 @@ struct drm_file { > > */ > > struct drm_prime_file_private prime; > > > > + /** > > + * @comm: Overridden task comm > > + * > > + * Accessed under override_lock > > + */ > > + char *override_comm; > > + > > + /** > > + * @cmdline: Overridden task cmdline > > + * > > + * Accessed under override_lock > > + */ > > + char *override_cmdline; > > + > > + /** > > + * @override_lock: Serialize access to override_comm and override_cmdline > > + */ > > + struct mutex override_lock; > > + > > I don't think this should go to drm just yet though. Only one driver can > make use of it so I'd leave it for later and print from msm_show_fdinfo > for now. This was my original approach but danvet asked that it be moved into drm for consistency across drivers. (And really, I want the in-flight amd and intel native-context stuff to motivate adding similar features to amdgpu/i915/xe.) BR, -R > Regards, > > Tvrtko > > > /* private: */ > > #if IS_ENABLED(CONFIG_DRM_LEGACY) > > unsigned long lock_count; /* DRI1 legacy lock count */
On 01/05/2023 17:58, Rob Clark wrote: > On Fri, Apr 28, 2023 at 4:05 AM Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: >> >> >> On 27/04/2023 18:53, Rob Clark wrote: >>> From: Rob Clark <robdclark@chromium.org> >>> >>> These are useful in particular for VM scenarios where the process which >>> has opened to drm device file is just a proxy for the real user in a VM >>> guest. >>> >>> Signed-off-by: Rob Clark <robdclark@chromium.org> >>> --- >>> Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++ >>> drivers/gpu/drm/drm_file.c | 15 +++++++++++++++ >>> include/drm/drm_file.h | 19 +++++++++++++++++++ >>> 3 files changed, 52 insertions(+) >>> >>> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst >>> index 58dc0d3f8c58..e4877cf8089c 100644 >>> --- a/Documentation/gpu/drm-usage-stats.rst >>> +++ b/Documentation/gpu/drm-usage-stats.rst >>> @@ -73,6 +73,24 @@ scope of each device, in which case `drm-pdev` shall be present as well. >>> Userspace should make sure to not double account any usage statistics by using >>> the above described criteria in order to associate data to individual clients. >>> >>> +- drm-comm-override: <valstr> >>> + >>> +Returns the client executable override string. Some drivers support letting >>> +userspace override this in cases where the userspace is simply a "proxy". >>> +Such as is the case with virglrenderer drm native context, where the host >>> +process is just forwarding command submission, etc, from guest userspace. >>> +This allows the proxy to make visible the executable name of the actual >>> +app in the VM guest. >>> + >>> +- drm-cmdline-override: <valstr> >>> + >>> +Returns the client cmdline override string. Some drivers support letting >>> +userspace override this in cases where the userspace is simply a "proxy". >>> +Such as is the case with virglrenderer drm native context, where the host >>> +process is just forwarding command submission, etc, from guest userspace. >>> +This allows the proxy to make visible the cmdline of the actual app in the >>> +VM guest. >> >> Perhaps it would be okay to save space here by not repeating the >> description, like: >> >> drm-comm-override: <valstr> >> drm-cmdline-override: <valstr> >> >> Long description blah blah... >> This allows the proxy to make visible the _executable name *and* command >> line_ blah blah.. >> >>> + >>> Utilization >>> ^^^^^^^^^^^ >>> >>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c >>> index 9321eb0bf020..d7514c313af1 100644 >>> --- a/drivers/gpu/drm/drm_file.c >>> +++ b/drivers/gpu/drm/drm_file.c >>> @@ -178,6 +178,8 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor) >>> spin_lock_init(&file->master_lookup_lock); >>> mutex_init(&file->event_read_lock); >>> >>> + mutex_init(&file->override_lock); >>> + >>> if (drm_core_check_feature(dev, DRIVER_GEM)) >>> drm_gem_open(dev, file); >>> >>> @@ -292,6 +294,8 @@ void drm_file_free(struct drm_file *file) >>> WARN_ON(!list_empty(&file->event_list)); >>> >>> put_pid(file->pid); >>> + kfree(file->override_comm); >>> + kfree(file->override_cmdline); >>> kfree(file); >>> } >>> >>> @@ -995,6 +999,17 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f) >>> PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); >>> } >>> >>> + mutex_lock(&file->override_lock); >> >> You could add a fast unlocked check before taking the mutex for no risk >> apart a transient false negative. For 99.9999% of userspace it would >> mean no pointless lock/unlock cycle. > > I'm not sure I get your point? This needs to be serialized against > userspace setting the override values if (file->override_comm || file->override_cmdline) { mutex_lock(&file->override_lock); if (file->override_comm) drm_printf(&p, "drm-comm-override:\t%s\n", file->override_comm); if (file->override_cmdline) drm_printf(&p, "drm-cmdline-override:\t%s\n", file->override_cmdline); mutext_unlock(&file->override_lock); } No risk apart for a transient false negative (which is immaterial for userspace since fdinfo reads are not ordered versus the override setting anyway) and 99.9% of deployments can get by not needing to pointlessly cycle the lock. > >> >>> + if (file->override_comm) { >>> + drm_printf(&p, "drm-comm-override:\t%s\n", >>> + file->override_comm); >>> + } >>> + if (file->override_cmdline) { >>> + drm_printf(&p, "drm-cmdline-override:\t%s\n", >>> + file->override_cmdline); >>> + } >>> + mutex_unlock(&file->override_lock); >>> + >>> if (dev->driver->show_fdinfo) >>> dev->driver->show_fdinfo(&p, file); >>> } >>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h >>> index 1339e925af52..604d05fa6f0c 100644 >>> --- a/include/drm/drm_file.h >>> +++ b/include/drm/drm_file.h >>> @@ -370,6 +370,25 @@ struct drm_file { >>> */ >>> struct drm_prime_file_private prime; >>> >>> + /** >>> + * @comm: Overridden task comm >>> + * >>> + * Accessed under override_lock >>> + */ >>> + char *override_comm; >>> + >>> + /** >>> + * @cmdline: Overridden task cmdline >>> + * >>> + * Accessed under override_lock >>> + */ >>> + char *override_cmdline; >>> + >>> + /** >>> + * @override_lock: Serialize access to override_comm and override_cmdline >>> + */ >>> + struct mutex override_lock; >>> + >> >> I don't think this should go to drm just yet though. Only one driver can >> make use of it so I'd leave it for later and print from msm_show_fdinfo >> for now. > > This was my original approach but danvet asked that it be moved into > drm for consistency across drivers. (And really, I want the in-flight > amd and intel native-context stuff to motivate adding similar features > to amdgpu/i915/xe.) IMO if implementation is not shared, not even by using helpers, I don't think data storage should be either, but it's not a deal breaker. Regards, Tvrtko > > BR, > -R > >> Regards, >> >> Tvrtko >> >>> /* private: */ >>> #if IS_ENABLED(CONFIG_DRM_LEGACY) >>> unsigned long lock_count; /* DRI1 legacy lock count */
In case you were waiting for me looking at the rest of the series, there was this reply from the previous round I can expand on. On 02/05/2023 08:50, Tvrtko Ursulin wrote: > > On 01/05/2023 17:58, Rob Clark wrote: >> On Fri, Apr 28, 2023 at 4:05 AM Tvrtko Ursulin >> <tvrtko.ursulin@linux.intel.com> wrote: >>> >>> >>> On 27/04/2023 18:53, Rob Clark wrote: >>>> From: Rob Clark <robdclark@chromium.org> >>>> >>>> These are useful in particular for VM scenarios where the process which >>>> has opened to drm device file is just a proxy for the real user in a VM >>>> guest. >>>> >>>> Signed-off-by: Rob Clark <robdclark@chromium.org> >>>> --- >>>> Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++ >>>> drivers/gpu/drm/drm_file.c | 15 +++++++++++++++ >>>> include/drm/drm_file.h | 19 +++++++++++++++++++ >>>> 3 files changed, 52 insertions(+) >>>> >>>> diff --git a/Documentation/gpu/drm-usage-stats.rst >>>> b/Documentation/gpu/drm-usage-stats.rst >>>> index 58dc0d3f8c58..e4877cf8089c 100644 >>>> --- a/Documentation/gpu/drm-usage-stats.rst >>>> +++ b/Documentation/gpu/drm-usage-stats.rst >>>> @@ -73,6 +73,24 @@ scope of each device, in which case `drm-pdev` >>>> shall be present as well. >>>> Userspace should make sure to not double account any usage >>>> statistics by using >>>> the above described criteria in order to associate data to >>>> individual clients. >>>> >>>> +- drm-comm-override: <valstr> >>>> + >>>> +Returns the client executable override string. Some drivers >>>> support letting >>>> +userspace override this in cases where the userspace is simply a >>>> "proxy". >>>> +Such as is the case with virglrenderer drm native context, where >>>> the host >>>> +process is just forwarding command submission, etc, from guest >>>> userspace. >>>> +This allows the proxy to make visible the executable name of the >>>> actual >>>> +app in the VM guest. >>>> + >>>> +- drm-cmdline-override: <valstr> >>>> + >>>> +Returns the client cmdline override string. Some drivers support >>>> letting >>>> +userspace override this in cases where the userspace is simply a >>>> "proxy". >>>> +Such as is the case with virglrenderer drm native context, where >>>> the host >>>> +process is just forwarding command submission, etc, from guest >>>> userspace. >>>> +This allows the proxy to make visible the cmdline of the actual app >>>> in the >>>> +VM guest. >>> >>> Perhaps it would be okay to save space here by not repeating the >>> description, like: >>> >>> drm-comm-override: <valstr> >>> drm-cmdline-override: <valstr> >>> >>> Long description blah blah... >>> This allows the proxy to make visible the _executable name *and* command >>> line_ blah blah.. >>> >>>> + >>>> Utilization >>>> ^^^^^^^^^^^ >>>> >>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c >>>> index 9321eb0bf020..d7514c313af1 100644 >>>> --- a/drivers/gpu/drm/drm_file.c >>>> +++ b/drivers/gpu/drm/drm_file.c >>>> @@ -178,6 +178,8 @@ struct drm_file *drm_file_alloc(struct drm_minor >>>> *minor) >>>> spin_lock_init(&file->master_lookup_lock); >>>> mutex_init(&file->event_read_lock); >>>> >>>> + mutex_init(&file->override_lock); >>>> + >>>> if (drm_core_check_feature(dev, DRIVER_GEM)) >>>> drm_gem_open(dev, file); >>>> >>>> @@ -292,6 +294,8 @@ void drm_file_free(struct drm_file *file) >>>> WARN_ON(!list_empty(&file->event_list)); >>>> >>>> put_pid(file->pid); >>>> + kfree(file->override_comm); >>>> + kfree(file->override_cmdline); >>>> kfree(file); >>>> } >>>> >>>> @@ -995,6 +999,17 @@ void drm_show_fdinfo(struct seq_file *m, struct >>>> file *f) >>>> PCI_SLOT(pdev->devfn), >>>> PCI_FUNC(pdev->devfn)); >>>> } >>>> >>>> + mutex_lock(&file->override_lock); >>> >>> You could add a fast unlocked check before taking the mutex for no risk >>> apart a transient false negative. For 99.9999% of userspace it would >>> mean no pointless lock/unlock cycle. >> >> I'm not sure I get your point? This needs to be serialized against >> userspace setting the override values > > if (file->override_comm || file->override_cmdline) { > mutex_lock(&file->override_lock); > if (file->override_comm) > drm_printf(&p, "drm-comm-override:\t%s\n", > file->override_comm); > if (file->override_cmdline) > drm_printf(&p, "drm-cmdline-override:\t%s\n", > file->override_cmdline); > mutext_unlock(&file->override_lock); > } > > No risk apart for a transient false negative (which is immaterial for > userspace since fdinfo reads are not ordered versus the override setting > anyway) and 99.9% of deployments can get by not needing to pointlessly > cycle the lock. This fast path bypass I think is worth it but up to you if you are really opposed. It's just that I don't see a point for cycling the mutex for nothing in majority of cases. >>> >>>> + if (file->override_comm) { >>>> + drm_printf(&p, "drm-comm-override:\t%s\n", >>>> + file->override_comm); >>>> + } >>>> + if (file->override_cmdline) { >>>> + drm_printf(&p, "drm-cmdline-override:\t%s\n", >>>> + file->override_cmdline); >>>> + } >>>> + mutex_unlock(&file->override_lock); >>>> + >>>> if (dev->driver->show_fdinfo) >>>> dev->driver->show_fdinfo(&p, file); >>>> } >>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h >>>> index 1339e925af52..604d05fa6f0c 100644 >>>> --- a/include/drm/drm_file.h >>>> +++ b/include/drm/drm_file.h >>>> @@ -370,6 +370,25 @@ struct drm_file { >>>> */ >>>> struct drm_prime_file_private prime; >>>> >>>> + /** >>>> + * @comm: Overridden task comm >>>> + * >>>> + * Accessed under override_lock >>>> + */ >>>> + char *override_comm; >>>> + >>>> + /** >>>> + * @cmdline: Overridden task cmdline >>>> + * >>>> + * Accessed under override_lock >>>> + */ >>>> + char *override_cmdline; >>>> + >>>> + /** >>>> + * @override_lock: Serialize access to override_comm and >>>> override_cmdline >>>> + */ >>>> + struct mutex override_lock; >>>> + >>> >>> I don't think this should go to drm just yet though. Only one driver can >>> make use of it so I'd leave it for later and print from msm_show_fdinfo >>> for now. >> >> This was my original approach but danvet asked that it be moved into >> drm for consistency across drivers. (And really, I want the in-flight >> amd and intel native-context stuff to motivate adding similar features >> to amdgpu/i915/xe.) > > IMO if implementation is not shared, not even by using helpers, I don't > think data storage should be either, but it's not a deal breaker. To summarise my thoughts on the patch (v4): I am not really keen on the split of data fields in common and no common implementation or helpers. For what the drm-usage-stats.rst are concerned it looks completely fine. And feature really will be useful in virtualised stacks. Code in this patch is also completely fine. Therefore you can have an r-b on those parts, but with reservations on whether it makes sense to put the fields under drm_file just yet. That should be fine under the r-b rules AFAIU. Ideally you can collect an ack from someone else too. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko > > Regards, > > Tvrtko > >> >> BR, >> -R >> >>> Regards, >>> >>> Tvrtko >>> >>>> /* private: */ >>>> #if IS_ENABLED(CONFIG_DRM_LEGACY) >>>> unsigned long lock_count; /* DRI1 legacy lock count */
On Thu, May 18, 2023 at 2:43 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > In case you were waiting for me looking at the rest of the series, there > was this reply from the previous round I can expand on. > > On 02/05/2023 08:50, Tvrtko Ursulin wrote: > > > > On 01/05/2023 17:58, Rob Clark wrote: > >> On Fri, Apr 28, 2023 at 4:05 AM Tvrtko Ursulin > >> <tvrtko.ursulin@linux.intel.com> wrote: > >>> > >>> > >>> On 27/04/2023 18:53, Rob Clark wrote: > >>>> From: Rob Clark <robdclark@chromium.org> > >>>> > >>>> These are useful in particular for VM scenarios where the process which > >>>> has opened to drm device file is just a proxy for the real user in a VM > >>>> guest. > >>>> > >>>> Signed-off-by: Rob Clark <robdclark@chromium.org> > >>>> --- > >>>> Documentation/gpu/drm-usage-stats.rst | 18 ++++++++++++++++++ > >>>> drivers/gpu/drm/drm_file.c | 15 +++++++++++++++ > >>>> include/drm/drm_file.h | 19 +++++++++++++++++++ > >>>> 3 files changed, 52 insertions(+) > >>>> > >>>> diff --git a/Documentation/gpu/drm-usage-stats.rst > >>>> b/Documentation/gpu/drm-usage-stats.rst > >>>> index 58dc0d3f8c58..e4877cf8089c 100644 > >>>> --- a/Documentation/gpu/drm-usage-stats.rst > >>>> +++ b/Documentation/gpu/drm-usage-stats.rst > >>>> @@ -73,6 +73,24 @@ scope of each device, in which case `drm-pdev` > >>>> shall be present as well. > >>>> Userspace should make sure to not double account any usage > >>>> statistics by using > >>>> the above described criteria in order to associate data to > >>>> individual clients. > >>>> > >>>> +- drm-comm-override: <valstr> > >>>> + > >>>> +Returns the client executable override string. Some drivers > >>>> support letting > >>>> +userspace override this in cases where the userspace is simply a > >>>> "proxy". > >>>> +Such as is the case with virglrenderer drm native context, where > >>>> the host > >>>> +process is just forwarding command submission, etc, from guest > >>>> userspace. > >>>> +This allows the proxy to make visible the executable name of the > >>>> actual > >>>> +app in the VM guest. > >>>> + > >>>> +- drm-cmdline-override: <valstr> > >>>> + > >>>> +Returns the client cmdline override string. Some drivers support > >>>> letting > >>>> +userspace override this in cases where the userspace is simply a > >>>> "proxy". > >>>> +Such as is the case with virglrenderer drm native context, where > >>>> the host > >>>> +process is just forwarding command submission, etc, from guest > >>>> userspace. > >>>> +This allows the proxy to make visible the cmdline of the actual app > >>>> in the > >>>> +VM guest. > >>> > >>> Perhaps it would be okay to save space here by not repeating the > >>> description, like: > >>> > >>> drm-comm-override: <valstr> > >>> drm-cmdline-override: <valstr> > >>> > >>> Long description blah blah... > >>> This allows the proxy to make visible the _executable name *and* command > >>> line_ blah blah.. > >>> > >>>> + > >>>> Utilization > >>>> ^^^^^^^^^^^ > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > >>>> index 9321eb0bf020..d7514c313af1 100644 > >>>> --- a/drivers/gpu/drm/drm_file.c > >>>> +++ b/drivers/gpu/drm/drm_file.c > >>>> @@ -178,6 +178,8 @@ struct drm_file *drm_file_alloc(struct drm_minor > >>>> *minor) > >>>> spin_lock_init(&file->master_lookup_lock); > >>>> mutex_init(&file->event_read_lock); > >>>> > >>>> + mutex_init(&file->override_lock); > >>>> + > >>>> if (drm_core_check_feature(dev, DRIVER_GEM)) > >>>> drm_gem_open(dev, file); > >>>> > >>>> @@ -292,6 +294,8 @@ void drm_file_free(struct drm_file *file) > >>>> WARN_ON(!list_empty(&file->event_list)); > >>>> > >>>> put_pid(file->pid); > >>>> + kfree(file->override_comm); > >>>> + kfree(file->override_cmdline); > >>>> kfree(file); > >>>> } > >>>> > >>>> @@ -995,6 +999,17 @@ void drm_show_fdinfo(struct seq_file *m, struct > >>>> file *f) > >>>> PCI_SLOT(pdev->devfn), > >>>> PCI_FUNC(pdev->devfn)); > >>>> } > >>>> > >>>> + mutex_lock(&file->override_lock); > >>> > >>> You could add a fast unlocked check before taking the mutex for no risk > >>> apart a transient false negative. For 99.9999% of userspace it would > >>> mean no pointless lock/unlock cycle. > >> > >> I'm not sure I get your point? This needs to be serialized against > >> userspace setting the override values > > > > if (file->override_comm || file->override_cmdline) { > > mutex_lock(&file->override_lock); > > if (file->override_comm) > > drm_printf(&p, "drm-comm-override:\t%s\n", > > file->override_comm); > > if (file->override_cmdline) > > drm_printf(&p, "drm-cmdline-override:\t%s\n", > > file->override_cmdline); > > mutext_unlock(&file->override_lock); > > } > > > > No risk apart for a transient false negative (which is immaterial for > > userspace since fdinfo reads are not ordered versus the override setting > > anyway) and 99.9% of deployments can get by not needing to pointlessly > > cycle the lock. > > This fast path bypass I think is worth it but up to you if you are > really opposed. It's just that I don't see a point for cycling the mutex > for nothing in majority of cases. I think it is a premature optimization.. an uncontended lock is "just" an atomic. Yes, atomics can be expensive in a hot path.. but in this case it is going to be lost in the noise. I did look a bit at gputop with `perf record` and it is very much not the problem. > >>> > >>>> + if (file->override_comm) { > >>>> + drm_printf(&p, "drm-comm-override:\t%s\n", > >>>> + file->override_comm); > >>>> + } > >>>> + if (file->override_cmdline) { > >>>> + drm_printf(&p, "drm-cmdline-override:\t%s\n", > >>>> + file->override_cmdline); > >>>> + } > >>>> + mutex_unlock(&file->override_lock); > >>>> + > >>>> if (dev->driver->show_fdinfo) > >>>> dev->driver->show_fdinfo(&p, file); > >>>> } > >>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > >>>> index 1339e925af52..604d05fa6f0c 100644 > >>>> --- a/include/drm/drm_file.h > >>>> +++ b/include/drm/drm_file.h > >>>> @@ -370,6 +370,25 @@ struct drm_file { > >>>> */ > >>>> struct drm_prime_file_private prime; > >>>> > >>>> + /** > >>>> + * @comm: Overridden task comm > >>>> + * > >>>> + * Accessed under override_lock > >>>> + */ > >>>> + char *override_comm; > >>>> + > >>>> + /** > >>>> + * @cmdline: Overridden task cmdline > >>>> + * > >>>> + * Accessed under override_lock > >>>> + */ > >>>> + char *override_cmdline; > >>>> + > >>>> + /** > >>>> + * @override_lock: Serialize access to override_comm and > >>>> override_cmdline > >>>> + */ > >>>> + struct mutex override_lock; > >>>> + > >>> > >>> I don't think this should go to drm just yet though. Only one driver can > >>> make use of it so I'd leave it for later and print from msm_show_fdinfo > >>> for now. > >> > >> This was my original approach but danvet asked that it be moved into > >> drm for consistency across drivers. (And really, I want the in-flight > >> amd and intel native-context stuff to motivate adding similar features > >> to amdgpu/i915/xe.) > > > > IMO if implementation is not shared, not even by using helpers, I don't > > think data storage should be either, but it's not a deal breaker. > > To summarise my thoughts on the patch (v4): > > I am not really keen on the split of data fields in common and no common > implementation or helpers. I can go either way on this.. it was danvet that suggested moving to drm_file to encourage more standardization. (But we can also land the meminfo parts of the series without this part.. it was just convenient for me to keep them in the same series to avoid conflicts) BR, -R > For what the drm-usage-stats.rst are concerned it looks completely fine. > And feature really will be useful in virtualised stacks. > > Code in this patch is also completely fine. > > Therefore you can have an r-b on those parts, but with reservations on > whether it makes sense to put the fields under drm_file just yet. That > should be fine under the r-b rules AFAIU. Ideally you can collect an ack > from someone else too. > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Regards, > > Tvrtko > > > > > Regards, > > > > Tvrtko > > > >> > >> BR, > >> -R > >> > >>> Regards, > >>> > >>> Tvrtko > >>> > >>>> /* private: */ > >>>> #if IS_ENABLED(CONFIG_DRM_LEGACY) > >>>> unsigned long lock_count; /* DRI1 legacy lock count */
diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst index 58dc0d3f8c58..e4877cf8089c 100644 --- a/Documentation/gpu/drm-usage-stats.rst +++ b/Documentation/gpu/drm-usage-stats.rst @@ -73,6 +73,24 @@ scope of each device, in which case `drm-pdev` shall be present as well. Userspace should make sure to not double account any usage statistics by using the above described criteria in order to associate data to individual clients. +- drm-comm-override: <valstr> + +Returns the client executable override string. Some drivers support letting +userspace override this in cases where the userspace is simply a "proxy". +Such as is the case with virglrenderer drm native context, where the host +process is just forwarding command submission, etc, from guest userspace. +This allows the proxy to make visible the executable name of the actual +app in the VM guest. + +- drm-cmdline-override: <valstr> + +Returns the client cmdline override string. Some drivers support letting +userspace override this in cases where the userspace is simply a "proxy". +Such as is the case with virglrenderer drm native context, where the host +process is just forwarding command submission, etc, from guest userspace. +This allows the proxy to make visible the cmdline of the actual app in the +VM guest. + Utilization ^^^^^^^^^^^ diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 9321eb0bf020..d7514c313af1 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -178,6 +178,8 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor) spin_lock_init(&file->master_lookup_lock); mutex_init(&file->event_read_lock); + mutex_init(&file->override_lock); + if (drm_core_check_feature(dev, DRIVER_GEM)) drm_gem_open(dev, file); @@ -292,6 +294,8 @@ void drm_file_free(struct drm_file *file) WARN_ON(!list_empty(&file->event_list)); put_pid(file->pid); + kfree(file->override_comm); + kfree(file->override_cmdline); kfree(file); } @@ -995,6 +999,17 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f) PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); } + mutex_lock(&file->override_lock); + if (file->override_comm) { + drm_printf(&p, "drm-comm-override:\t%s\n", + file->override_comm); + } + if (file->override_cmdline) { + drm_printf(&p, "drm-cmdline-override:\t%s\n", + file->override_cmdline); + } + mutex_unlock(&file->override_lock); + if (dev->driver->show_fdinfo) dev->driver->show_fdinfo(&p, file); } diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 1339e925af52..604d05fa6f0c 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -370,6 +370,25 @@ struct drm_file { */ struct drm_prime_file_private prime; + /** + * @comm: Overridden task comm + * + * Accessed under override_lock + */ + char *override_comm; + + /** + * @cmdline: Overridden task cmdline + * + * Accessed under override_lock + */ + char *override_cmdline; + + /** + * @override_lock: Serialize access to override_comm and override_cmdline + */ + struct mutex override_lock; + /* private: */ #if IS_ENABLED(CONFIG_DRM_LEGACY) unsigned long lock_count; /* DRI1 legacy lock count */