Re: [Mesa-dev] [PATCH v2 2/5] panfrost: Allocate shaders in their own BOs
On Fri, Aug 9, 2019 at 3:31 AM Tomeu Vizoso wrote:
>
> On Thu, 8 Aug 2019 at 16:19, Rob Herring wrote:
> >
> > On Wed, Aug 7, 2019 at 11:23 PM Tomeu Vizoso
> > wrote:
> > >
> > > On Thu, 8 Aug 2019 at 00:47, Rob Herring wrote:
> > > >
> > > > On Wed, Aug 7, 2019 at 2:37 AM Tomeu Vizoso
> > > > wrote:
> > > > >
> > > > > Instead of all shaders being stored in a single BO, have each shader
> > > > > in
> > > > > its own.
> > > > >
> > > > > This removes the need for a 16MB allocation per context, and allows us
> > > > > to place transient blend shaders in BOs marked as executable (before
> > > > > they were allocated in the transient pool, which shouldn't be
> > > > > executable).
> > > > >
> > > > > v2: - Store compiled blend shaders in a malloc'ed buffer, to avoid
> > > > > reading from GPU-accessible memory when patching (Alyssa).
> > > > > - Free struct panfrost_blend_shader (Alyssa).
> > > > > - Give the job a reference to regular shaders when emitting
> > > > > (Alyssa).
> > > > >
> > > > > Signed-off-by: Tomeu Vizoso
> > > >
> > > >
> > > > > diff --git a/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > > > b/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > > > index fba495c1dd69..7378d0a8abea 100644
> > > > > --- a/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > > > +++ b/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > > > @@ -84,11 +84,10 @@ panfrost_bo_cache_fetch(
> > > > > {
> > > > > struct list_head *bucket = pan_bucket(screen, size);
> > > > >
> > > > > -/* TODO: Honour flags? */
> > > > > -
> > > > > /* Iterate the bucket looking for something suitable */
> > > > > list_for_each_entry_safe(struct panfrost_bo, entry, bucket,
> > > > > link) {
> > > > > -if (entry->size >= size) {
> > > > > +if (entry->size >= size &&
> > > > > +entry->flags == flags) {
> > > >
> > > > This change probably warrants its own patch IMO.
> > >
> > > Agreed.
> > >
> > > > This is using the
> > > > untranslated flags, but I think it should be the 'translated_flags' as
> > > > those are the ones changing the allocation. For example, I don't think
> > > > there's any reason for DELAYED_MMAP to be used as a match criteria
> > > > (BTW, I'm also not sure if we can reclaim BOs if they remain mmap'ed).
> > > >
> > > > Another problem I see, if we have a 100MB buffer in the cache, would
> > > > we really want to hit on a 4KB allocation? Perhaps a 'entry->size * 2
> > > > < size' check.
> > >
> > > Yeah, as mentioned in the v1 discussion, we have plenty of room for
> > > improvements here, but the goal now is just to stop doing memory
> > > allocation so greedily that we reach OOM after launching a few GL
> > > clients.
> >
> > Sure. IMO, committing the BO cache without madvise was a mistake.
> > Without madvise, 2 instances of glmark will OOM.
>
> How can I test that? I just checked here and I'm running 10 instances
> of it within gnome-shell with 1GB still free (from a total of 2GB).
> This is with HEAP support, without it we'd be still allocating one
> 16MB buffer per context, but it's still not that bad.
>
> It used to be pretty bad when we were allocating gigantic buffers on
> context creation, just to be safe. But Mesa master now is much more
> careful with that and I think .
>
> > I should be able to
> > send out the patch for it today. I think it's going to need to disable
> > caching when madvise is not supported.
>
> Can you check if that would be still needed, please?
It definitely seems better now.
I think before I also had lots of debug dmsg going to disk and filling
the page cache. I tried reproducing that, but still seems okay now.
Rob
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 2/5] panfrost: Allocate shaders in their own BOs
On Thu, 8 Aug 2019 at 16:50, Rob Herring wrote: > > On Wed, Aug 7, 2019 at 5:47 PM Alyssa Rosenzweig > wrote: > > > > > This is using the > > > untranslated flags, but I think it should be the 'translated_flags' as > > > those are the ones changing the allocation. > > > > It's a little more complex than that. There some hypothetical > > untranslated flags that I would want to match on. For instance, future > > CPU read-only/write-only modifiers -- those affect the mmap (and need to > > be accounted for in the BO cache) but aren't specified as > > translated_flags to the kernel. > > I'll still argue that we shouldn't leave cached BOs mmap'ed so that > example would be mute. > > The more bits we have to match on, the less effective the BO cache > will be. Either we should use translated_flags or we should filter the > untranslated flags to the ones we care about. The latter would be more > flexible I guess. Yeah, there's lots to optimize still, fortunately :) Cheers, Tomeu ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 2/5] panfrost: Allocate shaders in their own BOs
On Thu, 8 Aug 2019 at 16:19, Rob Herring wrote:
>
> On Wed, Aug 7, 2019 at 11:23 PM Tomeu Vizoso
> wrote:
> >
> > On Thu, 8 Aug 2019 at 00:47, Rob Herring wrote:
> > >
> > > On Wed, Aug 7, 2019 at 2:37 AM Tomeu Vizoso
> > > wrote:
> > > >
> > > > Instead of all shaders being stored in a single BO, have each shader in
> > > > its own.
> > > >
> > > > This removes the need for a 16MB allocation per context, and allows us
> > > > to place transient blend shaders in BOs marked as executable (before
> > > > they were allocated in the transient pool, which shouldn't be
> > > > executable).
> > > >
> > > > v2: - Store compiled blend shaders in a malloc'ed buffer, to avoid
> > > > reading from GPU-accessible memory when patching (Alyssa).
> > > > - Free struct panfrost_blend_shader (Alyssa).
> > > > - Give the job a reference to regular shaders when emitting
> > > > (Alyssa).
> > > >
> > > > Signed-off-by: Tomeu Vizoso
> > >
> > >
> > > > diff --git a/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > > b/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > > index fba495c1dd69..7378d0a8abea 100644
> > > > --- a/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > > +++ b/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > > @@ -84,11 +84,10 @@ panfrost_bo_cache_fetch(
> > > > {
> > > > struct list_head *bucket = pan_bucket(screen, size);
> > > >
> > > > -/* TODO: Honour flags? */
> > > > -
> > > > /* Iterate the bucket looking for something suitable */
> > > > list_for_each_entry_safe(struct panfrost_bo, entry, bucket,
> > > > link) {
> > > > -if (entry->size >= size) {
> > > > +if (entry->size >= size &&
> > > > +entry->flags == flags) {
> > >
> > > This change probably warrants its own patch IMO.
> >
> > Agreed.
> >
> > > This is using the
> > > untranslated flags, but I think it should be the 'translated_flags' as
> > > those are the ones changing the allocation. For example, I don't think
> > > there's any reason for DELAYED_MMAP to be used as a match criteria
> > > (BTW, I'm also not sure if we can reclaim BOs if they remain mmap'ed).
> > >
> > > Another problem I see, if we have a 100MB buffer in the cache, would
> > > we really want to hit on a 4KB allocation? Perhaps a 'entry->size * 2
> > > < size' check.
> >
> > Yeah, as mentioned in the v1 discussion, we have plenty of room for
> > improvements here, but the goal now is just to stop doing memory
> > allocation so greedily that we reach OOM after launching a few GL
> > clients.
>
> Sure. IMO, committing the BO cache without madvise was a mistake.
> Without madvise, 2 instances of glmark will OOM.
How can I test that? I just checked here and I'm running 10 instances
of it within gnome-shell with 1GB still free (from a total of 2GB).
This is with HEAP support, without it we'd be still allocating one
16MB buffer per context, but it's still not that bad.
It used to be pretty bad when we were allocating gigantic buffers on
context creation, just to be safe. But Mesa master now is much more
careful with that and I think .
> I should be able to
> send out the patch for it today. I think it's going to need to disable
> caching when madvise is not supported.
Can you check if that would be still needed, please?
Thanks,
Tomeu
> Rob
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 2/5] panfrost: Allocate shaders in their own BOs
On Wed, Aug 7, 2019 at 5:47 PM Alyssa Rosenzweig wrote: > > > This is using the > > untranslated flags, but I think it should be the 'translated_flags' as > > those are the ones changing the allocation. > > It's a little more complex than that. There some hypothetical > untranslated flags that I would want to match on. For instance, future > CPU read-only/write-only modifiers -- those affect the mmap (and need to > be accounted for in the BO cache) but aren't specified as > translated_flags to the kernel. I'll still argue that we shouldn't leave cached BOs mmap'ed so that example would be mute. The more bits we have to match on, the less effective the BO cache will be. Either we should use translated_flags or we should filter the untranslated flags to the ones we care about. The latter would be more flexible I guess. Rob ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 2/5] panfrost: Allocate shaders in their own BOs
On Wed, Aug 7, 2019 at 11:23 PM Tomeu Vizoso wrote:
>
> On Thu, 8 Aug 2019 at 00:47, Rob Herring wrote:
> >
> > On Wed, Aug 7, 2019 at 2:37 AM Tomeu Vizoso
> > wrote:
> > >
> > > Instead of all shaders being stored in a single BO, have each shader in
> > > its own.
> > >
> > > This removes the need for a 16MB allocation per context, and allows us
> > > to place transient blend shaders in BOs marked as executable (before
> > > they were allocated in the transient pool, which shouldn't be
> > > executable).
> > >
> > > v2: - Store compiled blend shaders in a malloc'ed buffer, to avoid
> > > reading from GPU-accessible memory when patching (Alyssa).
> > > - Free struct panfrost_blend_shader (Alyssa).
> > > - Give the job a reference to regular shaders when emitting
> > > (Alyssa).
> > >
> > > Signed-off-by: Tomeu Vizoso
> >
> >
> > > diff --git a/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > b/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > index fba495c1dd69..7378d0a8abea 100644
> > > --- a/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > +++ b/src/gallium/drivers/panfrost/pan_bo_cache.c
> > > @@ -84,11 +84,10 @@ panfrost_bo_cache_fetch(
> > > {
> > > struct list_head *bucket = pan_bucket(screen, size);
> > >
> > > -/* TODO: Honour flags? */
> > > -
> > > /* Iterate the bucket looking for something suitable */
> > > list_for_each_entry_safe(struct panfrost_bo, entry, bucket,
> > > link) {
> > > -if (entry->size >= size) {
> > > +if (entry->size >= size &&
> > > +entry->flags == flags) {
> >
> > This change probably warrants its own patch IMO.
>
> Agreed.
>
> > This is using the
> > untranslated flags, but I think it should be the 'translated_flags' as
> > those are the ones changing the allocation. For example, I don't think
> > there's any reason for DELAYED_MMAP to be used as a match criteria
> > (BTW, I'm also not sure if we can reclaim BOs if they remain mmap'ed).
> >
> > Another problem I see, if we have a 100MB buffer in the cache, would
> > we really want to hit on a 4KB allocation? Perhaps a 'entry->size * 2
> > < size' check.
>
> Yeah, as mentioned in the v1 discussion, we have plenty of room for
> improvements here, but the goal now is just to stop doing memory
> allocation so greedily that we reach OOM after launching a few GL
> clients.
Sure. IMO, committing the BO cache without madvise was a mistake.
Without madvise, 2 instances of glmark will OOM. I should be able to
send out the patch for it today. I think it's going to need to disable
caching when madvise is not supported.
Rob
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 2/5] panfrost: Allocate shaders in their own BOs
On Thu, 8 Aug 2019 at 00:47, Rob Herring wrote:
>
> On Wed, Aug 7, 2019 at 2:37 AM Tomeu Vizoso
> wrote:
> >
> > Instead of all shaders being stored in a single BO, have each shader in
> > its own.
> >
> > This removes the need for a 16MB allocation per context, and allows us
> > to place transient blend shaders in BOs marked as executable (before
> > they were allocated in the transient pool, which shouldn't be
> > executable).
> >
> > v2: - Store compiled blend shaders in a malloc'ed buffer, to avoid
> > reading from GPU-accessible memory when patching (Alyssa).
> > - Free struct panfrost_blend_shader (Alyssa).
> > - Give the job a reference to regular shaders when emitting
> > (Alyssa).
> >
> > Signed-off-by: Tomeu Vizoso
>
>
> > diff --git a/src/gallium/drivers/panfrost/pan_bo_cache.c
> > b/src/gallium/drivers/panfrost/pan_bo_cache.c
> > index fba495c1dd69..7378d0a8abea 100644
> > --- a/src/gallium/drivers/panfrost/pan_bo_cache.c
> > +++ b/src/gallium/drivers/panfrost/pan_bo_cache.c
> > @@ -84,11 +84,10 @@ panfrost_bo_cache_fetch(
> > {
> > struct list_head *bucket = pan_bucket(screen, size);
> >
> > -/* TODO: Honour flags? */
> > -
> > /* Iterate the bucket looking for something suitable */
> > list_for_each_entry_safe(struct panfrost_bo, entry, bucket, link) {
> > -if (entry->size >= size) {
> > +if (entry->size >= size &&
> > +entry->flags == flags) {
>
> This change probably warrants its own patch IMO.
Agreed.
> This is using the
> untranslated flags, but I think it should be the 'translated_flags' as
> those are the ones changing the allocation. For example, I don't think
> there's any reason for DELAYED_MMAP to be used as a match criteria
> (BTW, I'm also not sure if we can reclaim BOs if they remain mmap'ed).
>
> Another problem I see, if we have a 100MB buffer in the cache, would
> we really want to hit on a 4KB allocation? Perhaps a 'entry->size * 2
> < size' check.
Yeah, as mentioned in the v1 discussion, we have plenty of room for
improvements here, but the goal now is just to stop doing memory
allocation so greedily that we reach OOM after launching a few GL
clients.
For 19.3 we could start tracking performance and other metrics such as
peak memory usage, etc.
Cheers,
Tomeu
>
> > /* This one works, splice it out of the cache */
> > list_del(>link);
> >
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 2/5] panfrost: Allocate shaders in their own BOs
> This is using the
> untranslated flags, but I think it should be the 'translated_flags' as
> those are the ones changing the allocation.
It's a little more complex than that. There some hypothetical
untranslated flags that I would want to match on. For instance, future
CPU read-only/write-only modifiers -- those affect the mmap (and need to
be accounted for in the BO cache) but aren't specified as
translated_flags to the kernel.
{ I'd like to experiment with read-only/write-only flags on certain
buffers as a sanity check for security/robustness reasons }
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 2/5] panfrost: Allocate shaders in their own BOs
On Wed, Aug 7, 2019 at 2:37 AM Tomeu Vizoso wrote:
>
> Instead of all shaders being stored in a single BO, have each shader in
> its own.
>
> This removes the need for a 16MB allocation per context, and allows us
> to place transient blend shaders in BOs marked as executable (before
> they were allocated in the transient pool, which shouldn't be
> executable).
>
> v2: - Store compiled blend shaders in a malloc'ed buffer, to avoid
> reading from GPU-accessible memory when patching (Alyssa).
> - Free struct panfrost_blend_shader (Alyssa).
> - Give the job a reference to regular shaders when emitting
> (Alyssa).
>
> Signed-off-by: Tomeu Vizoso
> diff --git a/src/gallium/drivers/panfrost/pan_bo_cache.c
> b/src/gallium/drivers/panfrost/pan_bo_cache.c
> index fba495c1dd69..7378d0a8abea 100644
> --- a/src/gallium/drivers/panfrost/pan_bo_cache.c
> +++ b/src/gallium/drivers/panfrost/pan_bo_cache.c
> @@ -84,11 +84,10 @@ panfrost_bo_cache_fetch(
> {
> struct list_head *bucket = pan_bucket(screen, size);
>
> -/* TODO: Honour flags? */
> -
> /* Iterate the bucket looking for something suitable */
> list_for_each_entry_safe(struct panfrost_bo, entry, bucket, link) {
> -if (entry->size >= size) {
> +if (entry->size >= size &&
> +entry->flags == flags) {
This change probably warrants its own patch IMO. This is using the
untranslated flags, but I think it should be the 'translated_flags' as
those are the ones changing the allocation. For example, I don't think
there's any reason for DELAYED_MMAP to be used as a match criteria
(BTW, I'm also not sure if we can reclaim BOs if they remain mmap'ed).
Another problem I see, if we have a 100MB buffer in the cache, would
we really want to hit on a 4KB allocation? Perhaps a 'entry->size * 2
< size' check.
> /* This one works, splice it out of the cache */
> list_del(>link);
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 2/5] panfrost: Allocate shaders in their own BOs
Patches 2-5 have my R-b, looks good! :)
On Wed, Aug 07, 2019 at 10:36:54AM +0200, Tomeu Vizoso wrote:
> Instead of all shaders being stored in a single BO, have each shader in
> its own.
>
> This removes the need for a 16MB allocation per context, and allows us
> to place transient blend shaders in BOs marked as executable (before
> they were allocated in the transient pool, which shouldn't be
> executable).
>
> v2: - Store compiled blend shaders in a malloc'ed buffer, to avoid
> reading from GPU-accessible memory when patching (Alyssa).
> - Free struct panfrost_blend_shader (Alyssa).
> - Give the job a reference to regular shaders when emitting
> (Alyssa).
>
> Signed-off-by: Tomeu Vizoso
> ---
> src/gallium/drivers/panfrost/pan_allocate.h | 2 +
> src/gallium/drivers/panfrost/pan_assemble.c | 5 ++-
> src/gallium/drivers/panfrost/pan_blend.h | 13 --
> src/gallium/drivers/panfrost/pan_blend_cso.c | 41 +--
> .../drivers/panfrost/pan_blend_shaders.c | 13 ++
> src/gallium/drivers/panfrost/pan_bo_cache.c | 5 +--
> src/gallium/drivers/panfrost/pan_context.c| 14 +--
> src/gallium/drivers/panfrost/pan_context.h| 3 +-
> src/gallium/drivers/panfrost/pan_drm.c| 5 ++-
> 9 files changed, 66 insertions(+), 35 deletions(-)
>
> diff --git a/src/gallium/drivers/panfrost/pan_allocate.h
> b/src/gallium/drivers/panfrost/pan_allocate.h
> index 8d925ee38a4c..0e06567d2069 100644
> --- a/src/gallium/drivers/panfrost/pan_allocate.h
> +++ b/src/gallium/drivers/panfrost/pan_allocate.h
> @@ -59,6 +59,8 @@ struct panfrost_bo {
> size_t size;
>
> int gem_handle;
> +
> +uint32_t flags;
> };
>
> struct panfrost_memory {
> diff --git a/src/gallium/drivers/panfrost/pan_assemble.c
> b/src/gallium/drivers/panfrost/pan_assemble.c
> index 15f77585a25c..dd877056d3b5 100644
> --- a/src/gallium/drivers/panfrost/pan_assemble.c
> +++ b/src/gallium/drivers/panfrost/pan_assemble.c
> @@ -43,6 +43,7 @@ panfrost_shader_compile(
> gl_shader_stage stage,
> struct panfrost_shader_state *state)
> {
> +struct panfrost_screen *screen = pan_screen(ctx->base.screen);
> uint8_t *dst;
>
> nir_shader *s;
> @@ -80,7 +81,9 @@ panfrost_shader_compile(
> * I bet someone just thought that would be a cute pun. At least,
> * that's how I'd do it. */
>
> -meta->shader = panfrost_upload(>shaders, dst, size) |
> program.first_tag;
> +state->bo = panfrost_drm_create_bo(screen, size,
> PAN_ALLOCATE_EXECUTE);
> +memcpy(state->bo->cpu, dst, size);
> +meta->shader = state->bo->gpu | program.first_tag;
>
> util_dynarray_fini();
>
> diff --git a/src/gallium/drivers/panfrost/pan_blend.h
> b/src/gallium/drivers/panfrost/pan_blend.h
> index a881e0945f48..a29353ff0014 100644
> --- a/src/gallium/drivers/panfrost/pan_blend.h
> +++ b/src/gallium/drivers/panfrost/pan_blend.h
> @@ -33,8 +33,10 @@
> /* An internal blend shader descriptor, from the compiler */
>
> struct panfrost_blend_shader {
> -/* The compiled shader in GPU memory */
> -struct panfrost_transfer shader;
> +struct panfrost_context *ctx;
> +
> +/* The compiled shader */
> +void *buffer;
>
> /* Byte count of the shader */
> unsigned size;
> @@ -53,8 +55,11 @@ struct panfrost_blend_shader {
> /* A blend shader descriptor ready for actual use */
>
> struct panfrost_blend_shader_final {
> -/* The upload, possibly to transient memory */
> -mali_ptr gpu;
> +/* The compiled shader in GPU memory, possibly patched */
> +struct panfrost_bo *bo;
> +
> +/* First instruction tag (for tagging the pointer) */
> +unsigned first_tag;
>
> /* Same meaning as panfrost_blend_shader */
> unsigned work_count;
> diff --git a/src/gallium/drivers/panfrost/pan_blend_cso.c
> b/src/gallium/drivers/panfrost/pan_blend_cso.c
> index f685b25b41b3..08a86980ca88 100644
> --- a/src/gallium/drivers/panfrost/pan_blend_cso.c
> +++ b/src/gallium/drivers/panfrost/pan_blend_cso.c
> @@ -151,11 +151,24 @@ panfrost_bind_blend_state(struct pipe_context *pipe,
> ctx->dirty |= PAN_DIRTY_FS;
> }
>
> +static void
> +panfrost_delete_blend_shader(struct hash_entry *entry)
> +{
> +struct panfrost_blend_shader *shader = (struct panfrost_blend_shader
> *)entry->data;
> +free(shader->buffer);
> +free(shader);
> +}
> +
> static void
> panfrost_delete_blend_state(struct pipe_context *pipe,
> -void *blend)
> +void *cso)
> {
> -/* TODO: Free shader binary? */
> +struct panfrost_blend_state *blend = (struct panfrost_blend_state *)
> cso;
> +
> +for (unsigned c = 0; c < 4; ++c) {
> +struct panfrost_blend_rt *rt = >rt[c];
> +
