| Message ID | 20220908181013.3214205-3-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/scheduler: track GPU active time per entity | expand |
Hi Lucas, On 08/09/2022 19:10, Lucas Stach wrote: > This exposes a accumulated GPU active time per client via the > fdinfo infrastructure. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 38 ++++++++++++++++++++++++++- > 1 file changed, 37 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > index b69edb40ae2a..11b1f11fcb58 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > @@ -22,6 +22,7 @@ > #include "etnaviv_gem.h" > #include "etnaviv_mmu.h" > #include "etnaviv_perfmon.h" > +#include "common.xml.h" > > /* > * DRM operations: > @@ -471,7 +472,42 @@ static const struct drm_ioctl_desc etnaviv_ioctls[] = { > ETNA_IOCTL(PM_QUERY_SIG, pm_query_sig, DRM_RENDER_ALLOW), > }; > > -DEFINE_DRM_GEM_FOPS(fops); > +static void etnaviv_fop_show_fdinfo(struct seq_file *m, struct file *f) > +{ > + struct drm_file *file = f->private_data; > + struct drm_device *dev = file->minor->dev; > + struct etnaviv_drm_private *priv = dev->dev_private; > + struct etnaviv_file_private *ctx = file->driver_priv; > + struct drm_printer p = drm_seq_file_printer(m); Any specific reason not to use seq_printf directly? (May be my ignorance.) > + int i; > + > + drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name); > + drm_printf(&p, "drm-client-id:\t%u\n", ctx->id); > + > + for (i = 0; i < ETNA_MAX_PIPES; i++) { > + struct etnaviv_gpu *gpu = priv->gpu[i]; > + char engine[8]; > + int cur = 0; Alignment renders odd in my client. > + > + if (!gpu) > + continue; I'd stick a comment in here to the effect of "For text output format description please see drm-usage-stats.rst!". Just to leave a breadcrumb putting some restraint on adding vendor specific things which may be already covered by the common spec. To help with common tools in the future as much as possible. > + > + if (gpu->identity.features & chipFeatures_PIPE_2D) > + cur = snprintf(engine, sizeof(engine), "2D"); > + if (gpu->identity.features & chipFeatures_PIPE_3D) > + cur = snprintf(engine + cur, sizeof(engine) - cur, > + "%s3D", cur ? "/" : ""); > + > + drm_printf(&p, "drm-engine-%s:\t%llu ns\n", engine, > + ctx->sched_entity[i].elapsed_ns); Two questions: 1) So you have max four pipes, each can be either only 2d, 3d, or 2d/3d? Can you have multiple of the same like 2x 3D? If you do, have you considered exporting one drm-engine-% together with drm-engine-capacity- for it? 2) Have you tried my, yet unmerged, vendor agnostic gputop tool with your changes? https://patchwork.freedesktop.org/series/102175/ It would be interesting to know if it works. Regards, Tvrtko > + } > +} > + > +static const struct file_operations fops = { > + .owner = THIS_MODULE, > + DRM_GEM_FOPS, > + .show_fdinfo = etnaviv_fop_show_fdinfo, > +}; > > static const struct drm_driver etnaviv_drm_driver = { > .driver_features = DRIVER_GEM | DRIVER_RENDER,
Hi Tvrtko, Am Freitag, dem 16.09.2022 um 10:31 +0100 schrieb Tvrtko Ursulin: > Hi Lucas, > > On 08/09/2022 19:10, Lucas Stach wrote: > > This exposes a accumulated GPU active time per client via the > > fdinfo infrastructure. > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 38 ++++++++++++++++++++++++++- > > 1 file changed, 37 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > index b69edb40ae2a..11b1f11fcb58 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > @@ -22,6 +22,7 @@ > > #include "etnaviv_gem.h" > > #include "etnaviv_mmu.h" > > #include "etnaviv_perfmon.h" > > +#include "common.xml.h" > > > > /* > > * DRM operations: > > @@ -471,7 +472,42 @@ static const struct drm_ioctl_desc etnaviv_ioctls[] = { > > ETNA_IOCTL(PM_QUERY_SIG, pm_query_sig, DRM_RENDER_ALLOW), > > }; > > > > -DEFINE_DRM_GEM_FOPS(fops); > > +static void etnaviv_fop_show_fdinfo(struct seq_file *m, struct file *f) > > +{ > > + struct drm_file *file = f->private_data; > > + struct drm_device *dev = file->minor->dev; > > + struct etnaviv_drm_private *priv = dev->dev_private; > > + struct etnaviv_file_private *ctx = file->driver_priv; > > + struct drm_printer p = drm_seq_file_printer(m); > > Any specific reason not to use seq_printf directly? (May be my ignorance.) > Not really, I just followed what msm was doing here. > > + int i; > > + > > + drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name); > > + drm_printf(&p, "drm-client-id:\t%u\n", ctx->id); > > + > > + for (i = 0; i < ETNA_MAX_PIPES; i++) { > > + struct etnaviv_gpu *gpu = priv->gpu[i]; > > + char engine[8]; > > + int cur = 0; > > Alignment renders odd in my client. I'll check that, might have messed up here. > > > + > > + if (!gpu) > > + continue; > > I'd stick a comment in here to the effect of "For text output format > description please see drm-usage-stats.rst!". > > Just to leave a breadcrumb putting some restraint on adding vendor > specific things which may be already covered by the common spec. To help > with common tools in the future as much as possible. Yea, it was pretty to clear to me that we want the common format as much as possible when writing the patches, but it's a good idea to add a pointer for the future reader. > > > + > > + if (gpu->identity.features & chipFeatures_PIPE_2D) > > + cur = snprintf(engine, sizeof(engine), "2D"); > > + if (gpu->identity.features & chipFeatures_PIPE_3D) > > + cur = snprintf(engine + cur, sizeof(engine) - cur, > > + "%s3D", cur ? "/" : ""); > > + > > + drm_printf(&p, "drm-engine-%s:\t%llu ns\n", engine, > > + ctx->sched_entity[i].elapsed_ns); > > Two questions: > > 1) > So you have max four pipes, each can be either only 2d, 3d, or 2d/3d? > Can you have multiple of the same like 2x 3D? If you do, have you > considered exporting one drm-engine-% together with drm-engine-capacity- > for it? > The four pipes is a arbitrary driver limit. Etnaviv is a bit special in that it collects all Vivante GPUs present in a system into a single DRM device, each of those GPUs can be either 2D, 3D or a combined core with both 2D and 3D capabilities. In theory there could be multiple GPUs of each kind, but for now all real SoC designs we've come across only had a single one of each kind. When we add support for a SoC that has multiple GPUs of one kind, I think exposing drm-engine-capacity, together with hooking them up to the load balancing in the scheduler is the right thing to do. > 2) > Have you tried my, yet unmerged, vendor agnostic gputop tool with your > changes? > > https://patchwork.freedesktop.org/series/102175/ > > It would be interesting to know if it works. > Yes, I did when working on this series. I had some crashes related to (I believe) double frees in the DRM client discovery, which I hadn't had time to investigate further. Seems there is a race, as I couldn't reproduce the crash when running with valgrind. Other than that, the tool works for exposing the per-client GPU load on etnaviv. Regards, Lucas > Regards, > > Tvrtko > > > + } > > +} > > + > > +static const struct file_operations fops = { > > + .owner = THIS_MODULE, > > + DRM_GEM_FOPS, > > + .show_fdinfo = etnaviv_fop_show_fdinfo, > > +}; > > > > static const struct drm_driver etnaviv_drm_driver = { > > .driver_features = DRIVER_GEM | DRIVER_RENDER,
On 16/09/2022 10:50, Lucas Stach wrote: > Hi Tvrtko, > > Am Freitag, dem 16.09.2022 um 10:31 +0100 schrieb Tvrtko Ursulin: >> Hi Lucas, >> >> On 08/09/2022 19:10, Lucas Stach wrote: >>> This exposes a accumulated GPU active time per client via the >>> fdinfo infrastructure. >>> >>> Signed-off-by: Lucas Stach <l.stach@pengutronix.de> >>> --- >>> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 38 ++++++++++++++++++++++++++- >>> 1 file changed, 37 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c >>> index b69edb40ae2a..11b1f11fcb58 100644 >>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c >>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c >>> @@ -22,6 +22,7 @@ >>> #include "etnaviv_gem.h" >>> #include "etnaviv_mmu.h" >>> #include "etnaviv_perfmon.h" >>> +#include "common.xml.h" >>> >>> /* >>> * DRM operations: >>> @@ -471,7 +472,42 @@ static const struct drm_ioctl_desc etnaviv_ioctls[] = { >>> ETNA_IOCTL(PM_QUERY_SIG, pm_query_sig, DRM_RENDER_ALLOW), >>> }; >>> >>> -DEFINE_DRM_GEM_FOPS(fops); >>> +static void etnaviv_fop_show_fdinfo(struct seq_file *m, struct file *f) >>> +{ >>> + struct drm_file *file = f->private_data; >>> + struct drm_device *dev = file->minor->dev; >>> + struct etnaviv_drm_private *priv = dev->dev_private; >>> + struct etnaviv_file_private *ctx = file->driver_priv; >>> + struct drm_printer p = drm_seq_file_printer(m); >> >> Any specific reason not to use seq_printf directly? (May be my ignorance.) >> > Not really, I just followed what msm was doing here. Right, no strong feelings either way I just did not see the need to wrap it for this use case. >>> + int i; >>> + >>> + drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name); >>> + drm_printf(&p, "drm-client-id:\t%u\n", ctx->id); >>> + >>> + for (i = 0; i < ETNA_MAX_PIPES; i++) { >>> + struct etnaviv_gpu *gpu = priv->gpu[i]; >>> + char engine[8]; >>> + int cur = 0; >> >> Alignment renders odd in my client. > > I'll check that, might have messed up here. >> >>> + >>> + if (!gpu) >>> + continue; >> >> I'd stick a comment in here to the effect of "For text output format >> description please see drm-usage-stats.rst!". >> >> Just to leave a breadcrumb putting some restraint on adding vendor >> specific things which may be already covered by the common spec. To help >> with common tools in the future as much as possible. > > Yea, it was pretty to clear to me that we want the common format as > much as possible when writing the patches, but it's a good idea to add > a pointer for the future reader. Thanks! >>> + >>> + if (gpu->identity.features & chipFeatures_PIPE_2D) >>> + cur = snprintf(engine, sizeof(engine), "2D"); >>> + if (gpu->identity.features & chipFeatures_PIPE_3D) >>> + cur = snprintf(engine + cur, sizeof(engine) - cur, >>> + "%s3D", cur ? "/" : ""); >>> + >>> + drm_printf(&p, "drm-engine-%s:\t%llu ns\n", engine, >>> + ctx->sched_entity[i].elapsed_ns); >> >> Two questions: >> >> 1) >> So you have max four pipes, each can be either only 2d, 3d, or 2d/3d? >> Can you have multiple of the same like 2x 3D? If you do, have you >> considered exporting one drm-engine-% together with drm-engine-capacity- >> for it? >> > The four pipes is a arbitrary driver limit. Etnaviv is a bit special in > that it collects all Vivante GPUs present in a system into a single DRM > device, each of those GPUs can be either 2D, 3D or a combined core with > both 2D and 3D capabilities. In theory there could be multiple GPUs of > each kind, but for now all real SoC designs we've come across only had > a single one of each kind. > > When we add support for a SoC that has multiple GPUs of one kind, I > think exposing drm-engine-capacity, together with hooking them up to > the load balancing in the scheduler is the right thing to do. > >> 2) >> Have you tried my, yet unmerged, vendor agnostic gputop tool with your >> changes? >> >> https://patchwork.freedesktop.org/series/102175/ >> >> It would be interesting to know if it works. >> > Yes, I did when working on this series. I had some crashes related to > (I believe) double frees in the DRM client discovery, which I hadn't > had time to investigate further. Seems there is a race, as I couldn't > reproduce the crash when running with valgrind. > > Other than that, the tool works for exposing the per-client GPU load on > etnaviv. Cool, at least some success. Out of curiosity what is the planned consumer in etnaviv landscape? Regards, Tvrtko
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index b69edb40ae2a..11b1f11fcb58 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -22,6 +22,7 @@ #include "etnaviv_gem.h" #include "etnaviv_mmu.h" #include "etnaviv_perfmon.h" +#include "common.xml.h" /* * DRM operations: @@ -471,7 +472,42 @@ static const struct drm_ioctl_desc etnaviv_ioctls[] = { ETNA_IOCTL(PM_QUERY_SIG, pm_query_sig, DRM_RENDER_ALLOW), }; -DEFINE_DRM_GEM_FOPS(fops); +static void etnaviv_fop_show_fdinfo(struct seq_file *m, struct file *f) +{ + struct drm_file *file = f->private_data; + struct drm_device *dev = file->minor->dev; + struct etnaviv_drm_private *priv = dev->dev_private; + struct etnaviv_file_private *ctx = file->driver_priv; + struct drm_printer p = drm_seq_file_printer(m); + int i; + + drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name); + drm_printf(&p, "drm-client-id:\t%u\n", ctx->id); + + for (i = 0; i < ETNA_MAX_PIPES; i++) { + struct etnaviv_gpu *gpu = priv->gpu[i]; + char engine[8]; + int cur = 0; + + if (!gpu) + continue; + + if (gpu->identity.features & chipFeatures_PIPE_2D) + cur = snprintf(engine, sizeof(engine), "2D"); + if (gpu->identity.features & chipFeatures_PIPE_3D) + cur = snprintf(engine + cur, sizeof(engine) - cur, + "%s3D", cur ? "/" : ""); + + drm_printf(&p, "drm-engine-%s:\t%llu ns\n", engine, + ctx->sched_entity[i].elapsed_ns); + } +} + +static const struct file_operations fops = { + .owner = THIS_MODULE, + DRM_GEM_FOPS, + .show_fdinfo = etnaviv_fop_show_fdinfo, +}; static const struct drm_driver etnaviv_drm_driver = { .driver_features = DRIVER_GEM | DRIVER_RENDER,
This exposes a accumulated GPU active time per client via the fdinfo infrastructure. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 38 ++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-)