On Tue, Apr 18, 2023 at 1:53 AM Tvrtko Ursulin
<tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
>
>
> On 17/04/2023 21:12, Rob Clark wrote:
> > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> >
> > Normally this would be the same information that can be obtained in
> > other ways. But in some cases the process opening the drm fd is merely
> > a sort of proxy for the actual process using the GPU. This is the case
> > for guest VM processes using the GPU via virglrenderer, in which case
> > the msm native-context renderer in virglrenderer overrides the comm/
> > cmdline to be the guest process's values.
> >
> > Exposing this via fdinfo allows tools like gputop to show something more
> > meaningful than just a bunch of "pcivirtio-gpu" users.
>
> You also later expanded with:
>
> """
> I should have also mentioned, in the VM/proxy scenario we have a
> single process with separate drm_file's for each guest VM process. So
> it isn't an option to just change the proxy process's name to match
> the client.
> """
>
> So how does that work - this single process temporarily changes it's
> name for each drm fd it opens and creates a context or it is actually in
> the native context protocol?
It is part of the protocol, the mesa driver in the VM sends[1] this
info to the native-context "shim" in host userspace which uses the
SET_PARAM ioctl to pass this to the kernel. In the host userspace
there is just a single process (you see the host PID below) but it
does a separate open() of the drm dev for each guest process (so that
they each have their own GPU address space for isolation):
DRM minor 128
PID MEM ACTIV NAME gpu
5297 200M 82M com.mojang.minecr |██████████████▏ |
1859 199M 0B chrome |█▉ |
5297 64M 9M surfaceflinger | |
5297 12M 0B org.chromium.arc. | |
5297 12M 0B com.android.syste | |
5297 12M 0B org.chromium.arc. | |
5297 26M 0B com.google.androi | |
5297 65M 0B system_server | |
[1] https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/drm/msm/msm_proto.h#L326
[2] https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/drm/msm/msm_renderer.c#L1050
> >
> > Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> > ---
> > Documentation/gpu/drm-usage-stats.rst | 8 ++++++++
> > drivers/gpu/drm/msm/msm_gpu.c | 14 ++++++++++++++
> > 2 files changed, 22 insertions(+)
> >
> > diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> > index 8e00d53231e0..bc90bed455e3 100644
> > --- a/Documentation/gpu/drm-usage-stats.rst
> > +++ b/Documentation/gpu/drm-usage-stats.rst
> > @@ -148,6 +148,14 @@ percentage utilization of the engine, whereas drm-engine-<keystr> only reflects
> > time active without considering what frequency the engine is operating as a
> > percentage of it's maximum frequency.
> >
> > +- drm-comm: <valstr>
> > +
> > +Returns the clients executable path.
>
> Full path and not just current->comm? In this case probably give it a
> more descriptive name here.
>
> drm-client-executable
> drm-client-command-line
>
> So we stay in the drm-client- namespace?
>
> Or if the former is absolute path could one key be enough for both?
>
> drm-client-command-line: /path/to/executable --arguments
comm and cmdline can be different. Android seems to change the comm to
the apk name, for example (and w/ the zygote stuff cmdline isn't
really a thing)
I guess it could be drm-client-comm and drm-client-cmdline? Although
comm/cmdline aren't the best names, they are just following what the
kernel calls them elsewhere.
> > +
> > +- drm-cmdline: <valstr>
> > +
> > +Returns the clients cmdline.
>
> I think drm-usage-stats.rst text should provide some more text with
> these two. To precisely define their content and outline the use case
> under which driver authors may want to add them, and fdinfo consumer
> therefore expect to see them. Just so everything is completely clear and
> people do not start adding them for drivers which do not support native
> context (or like).
I really was just piggy-backing on existing comm/cmdline.. but I'll
try to write up something better.
I think it maybe should not be limited just to native context.. for
ex. if the browser did somehow manage to create different displays
associated with different drm_file instances (I guess it would have to
use gbm to do this?) it would be nice to see browser tab names.
> But on the overall it sounds reasonable to me - it would be really cool
> to not just see pcivirtio-gpu as you say. Even if the standard virtiogpu
> use case (not native context) could show real users.
For vrend/virgl, we'd first need to solve the issue that there is just
a single drm_file for all guest processes. But really, just don't use
virgl. (I mean, like seriously, would you put a gl driver in the
kernel? Vrend has access to all guest memory, so this is essentially
what you have with virgl. This is just not a sane thing to do.) The
only "valid" reason for not doing native-context is if you don't have
the src code for your UMD to be able to modify it to talk
native-context to virtgpu in the guest. ;-)
BR,
-R
> Regards,
>
> Tvrtko
>
> > +
> > Implementation Details
> > ======================
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > index f0f4f845c32d..1150dcbf28aa 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > @@ -148,12 +148,26 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
> > return 0;
> > }
> >
> > +static void get_comm_cmdline(struct msm_file_private *ctx, char **comm, char **cmd);
> > +
> > void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
> > struct drm_printer *p)
> > {
> > + char *comm, *cmdline;
> > +
> > + get_comm_cmdline(ctx, &comm, &cmdline);
> > +
> > drm_printf(p, "drm-engine-gpu:\t%llu ns\n", ctx->elapsed_ns);
> > drm_printf(p, "drm-cycles-gpu:\t%llu\n", ctx->cycles);
> > drm_printf(p, "drm-maxfreq-gpu:\t%u Hz\n", gpu->fast_rate);
> > +
> > + if (comm)
> > + drm_printf(p, "drm-comm:\t%s\n", comm);
> > + if (cmdline)
> > + drm_printf(p, "drm-cmdline:\t%s\n", cmdline);
> > +
> > + kfree(comm);
> > + kfree(cmdline);
> > }
> >
> > int msm_gpu_hw_init(struct msm_gpu *gpu)
