Re: [Mesa-dev] [PATCH 4/4] anv/cmd_buffer: only emit state base address if the address changes
On Mon, 2018-07-02 at 08:23 -0500, Jason Ekstrand wrote:
> On July 2, 2018 01:09:38 Iago Toral wrote:
>
> > On Sun, 2018-07-01 at 18:30 -0500, Jason Ekstrand wrote:
> > > On June 29, 2018 03:11:00 Iago Toral Quiroga
> > > wrote:
> > >
> > > > ---
> > > > src/intel/vulkan/anv_private.h | 5 +
> > > > src/intel/vulkan/genX_cmd_buffer.c | 12 +++-
> > > > 2 files changed, 12 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/src/intel/vulkan/anv_private.h
> > > > b/src/intel/vulkan/anv_private.h
> > > > index 510471da602..1a9ab7013f2 100644
> > > > --- a/src/intel/vulkan/anv_private.h
> > > > +++ b/src/intel/vulkan/anv_private.h
> > > > @@ -1989,6 +1989,11 @@ struct anv_cmd_state {
> > > > * is one of the states in render_pass_states.
> > > > */
> > > > struct
> > > > anv_state null_surface_state;
> > > > +
> > > > + /**
> > > > +* Current state base address.
> > > > +*/
> > > > + struct
> > > > anv_address base_state_address;
> > > > };
> > > >
> > > > struct anv_cmd_pool {
> > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > > > b/src/intel/vulkan/genX_cmd_buffer.c
> > > > index 611311904e6..2847e0b30c9 100644
> > > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > > @@ -67,6 +67,12 @@
> > > > genX(cmd_buffer_emit_state_base_address)(struct
> > > > anv_cmd_buffer *cmd_buffer)
> > > > {
> > > > struct anv_device *device = cmd_buffer->device;
> > > >
> > > > + struct anv_address new_base_address =
> > > > + anv_cmd_buffer_surface_base_address(cmd_buffer);
> > > > + if (new_base_address.bo == cmd_buffer-
> > > > > state.base_state_address.bo &&
> > > >
> > > > + new_base_address.offset == cmd_buffer-
> > > > > state.base_state_address.offset)
> > > >
> > > > + return;
> > > > +
> > > > /* If we are emitting a new state base address we probably need
> > > > to re-emit
> > > > * binding tables.
> > > > */
> > > > @@ -90,8 +96,7 @@
> > > > genX(cmd_buffer_emit_state_base_address)(struct
> > > > anv_cmd_buffer *cmd_buffer)
> > > > sba.GeneralStateMemoryObjectControlState = GENX(MOCS);
> > > > sba.GeneralStateBaseAddressModifyEnable = true;
> > > >
> > > > - sba.SurfaceStateBaseAddress =
> > > > - anv_cmd_buffer_surface_base_address(cmd_buffer);
> > > > + sba.SurfaceStateBaseAddress = new_base_address;
> > > > sba.SurfaceStateMemoryObjectControlState = GENX(MOCS);
> > > > sba.SurfaceStateBaseAddressModifyEnable = true;
> > > >
> > > > @@ -1521,9 +1526,6 @@ genX(CmdExecuteCommands)(
> > > > /* Each of the secondary command buffers will use its own state
> > > > base
> > > > * address. We need to re-emit state base address for the
> > > > primary after
> > > > * all of the secondaries are done.
> > > > -*
> > > > -* TODO: Maybe we want to make this a dirty bit to avoid
> > > > extra
> > > > state base
> > > > -* address calls?
> > >
> > > I don't think this is correct. When a secondary executes, we
> > > have
> > > to
> > > reemit STATE_BASE_ADDRESS because the secondary used it's own and
> > > we
> > > need
> > > to set it back for the primary. The comment above was saying that
> > > we
> > > can
> > > probably avoid it if we have a bunch of ExecuteCommands calls
> > > back to
> > > back
> > > or if the last thing in the batch is a call out to a secondary.
> > > As is, I
> > > think this patch will cause problems in the case where the client
> > > uses a
> > > secondary followed by rendering in the primary. Have I missed
> > > something?
> >
> > I shouldn't remove the comment since this patche doesn't address
> > that
> > TODO, we still emit the state base address for the primary below,
> > the
> > only change in here is that if the base state address of the
> > primary is
> > the same as the one for the secondaries we won't actually emit the
> > state packet, but that should be fine. Maybe you thought I was
> > removing
> > the line below?
>
> The problem is that it never will be the same. The secondary always
> allocate a new binding table pool and re-emit STATE_BASE_ADDRESS so I
> don't
> think we're actually saving anything.
Ok, in that case I agree it won't help anything, thanks for clarifying.
Let's drop this patch then.
Iago
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] anv/cmd_buffer: only emit state base address if the address changes
On July 2, 2018 01:09:38 Iago Toral wrote:
On Sun, 2018-07-01 at 18:30 -0500, Jason Ekstrand wrote:
On June 29, 2018 03:11:00 Iago Toral Quiroga
wrote:
---
src/intel/vulkan/anv_private.h | 5 +
src/intel/vulkan/genX_cmd_buffer.c | 12 +++-
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/src/intel/vulkan/anv_private.h
b/src/intel/vulkan/anv_private.h
index 510471da602..1a9ab7013f2 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -1989,6 +1989,11 @@ struct anv_cmd_state {
* is one of the states in render_pass_states.
*/
struct anv_state null_surface_state;
+
+ /**
+* Current state base address.
+*/
+ struct
anv_address base_state_address;
};
struct anv_cmd_pool {
diff --git a/src/intel/vulkan/genX_cmd_buffer.c
b/src/intel/vulkan/genX_cmd_buffer.c
index 611311904e6..2847e0b30c9 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -67,6 +67,12 @@ genX(cmd_buffer_emit_state_base_address)(struct
anv_cmd_buffer *cmd_buffer)
{
struct anv_device *device = cmd_buffer->device;
+ struct anv_address new_base_address =
+ anv_cmd_buffer_surface_base_address(cmd_buffer);
+ if (new_base_address.bo == cmd_buffer-
state.base_state_address.bo &&
+ new_base_address.offset == cmd_buffer-
state.base_state_address.offset)
+ return;
+
/* If we are emitting a new state base address we probably need
to re-emit
* binding tables.
*/
@@ -90,8 +96,7 @@ genX(cmd_buffer_emit_state_base_address)(struct
anv_cmd_buffer *cmd_buffer)
sba.GeneralStateMemoryObjectControlState = GENX(MOCS);
sba.GeneralStateBaseAddressModifyEnable = true;
- sba.SurfaceStateBaseAddress =
- anv_cmd_buffer_surface_base_address(cmd_buffer);
+ sba.SurfaceStateBaseAddress = new_base_address;
sba.SurfaceStateMemoryObjectControlState = GENX(MOCS);
sba.SurfaceStateBaseAddressModifyEnable = true;
@@ -1521,9 +1526,6 @@ genX(CmdExecuteCommands)(
/* Each of the secondary command buffers will use its own state
base
* address. We need to re-emit state base address for the
primary after
* all of the secondaries are done.
-*
-* TODO: Maybe we want to make this a dirty bit to avoid extra
state base
-* address calls?
I don't think this is correct. When a secondary executes, we have
to
reemit STATE_BASE_ADDRESS because the secondary used it's own and we
need
to set it back for the primary. The comment above was saying that we
can
probably avoid it if we have a bunch of ExecuteCommands calls back to
back
or if the last thing in the batch is a call out to a secondary.
As is, I
think this patch will cause problems in the case where the client
uses a
secondary followed by rendering in the primary. Have I missed
something?
I shouldn't remove the comment since this patche doesn't address that
TODO, we still emit the state base address for the primary below, the
only change in here is that if the base state address of the primary is
the same as the one for the secondaries we won't actually emit the
state packet, but that should be fine. Maybe you thought I was removing
the line below?
The problem is that it never will be the same. The secondary always
allocate a new binding table pool and re-emit STATE_BASE_ADDRESS so I don't
think we're actually saving anything.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] anv/cmd_buffer: only emit state base address if the address changes
On Sun, 2018-07-01 at 18:30 -0500, Jason Ekstrand wrote:
> On June 29, 2018 03:11:00 Iago Toral Quiroga
> wrote:
>
> > ---
> > src/intel/vulkan/anv_private.h | 5 +
> > src/intel/vulkan/genX_cmd_buffer.c | 12 +++-
> > 2 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_private.h
> > b/src/intel/vulkan/anv_private.h
> > index 510471da602..1a9ab7013f2 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -1989,6 +1989,11 @@ struct anv_cmd_state {
> > * is one of the states in render_pass_states.
> > */
> >struct anv_state null_surface_state;
> > +
> > + /**
> > +* Current state base address.
> > +*/
> > + struct
> > anv_address base_state_address;
> > };
> >
> > struct anv_cmd_pool {
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > index 611311904e6..2847e0b30c9 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -67,6 +67,12 @@ genX(cmd_buffer_emit_state_base_address)(struct
> > anv_cmd_buffer *cmd_buffer)
> > {
> >struct anv_device *device = cmd_buffer->device;
> >
> > + struct anv_address new_base_address =
> > + anv_cmd_buffer_surface_base_address(cmd_buffer);
> > + if (new_base_address.bo == cmd_buffer-
> > >state.base_state_address.bo &&
> > + new_base_address.offset == cmd_buffer-
> > >state.base_state_address.offset)
> > + return;
> > +
> >/* If we are emitting a new state base address we probably need
> > to re-emit
> > * binding tables.
> > */
> > @@ -90,8 +96,7 @@ genX(cmd_buffer_emit_state_base_address)(struct
> > anv_cmd_buffer *cmd_buffer)
> > sba.GeneralStateMemoryObjectControlState = GENX(MOCS);
> > sba.GeneralStateBaseAddressModifyEnable = true;
> >
> > - sba.SurfaceStateBaseAddress =
> > - anv_cmd_buffer_surface_base_address(cmd_buffer);
> > + sba.SurfaceStateBaseAddress = new_base_address;
> > sba.SurfaceStateMemoryObjectControlState = GENX(MOCS);
> > sba.SurfaceStateBaseAddressModifyEnable = true;
> >
> > @@ -1521,9 +1526,6 @@ genX(CmdExecuteCommands)(
> >/* Each of the secondary command buffers will use its own state
> > base
> > * address. We need to re-emit state base address for the
> > primary after
> > * all of the secondaries are done.
> > -*
> > -* TODO: Maybe we want to make this a dirty bit to avoid extra
> > state base
> > -* address calls?
>
> I don't think this is correct. When a secondary executes, we have
> to
> reemit STATE_BASE_ADDRESS because the secondary used it's own and we
> need
> to set it back for the primary. The comment above was saying that we
> can
> probably avoid it if we have a bunch of ExecuteCommands calls back to
> back
> or if the last thing in the batch is a call out to a secondary.
> As is, I
> think this patch will cause problems in the case where the client
> uses a
> secondary followed by rendering in the primary. Have I missed
> something?
I shouldn't remove the comment since this patche doesn't address that
TODO, we still emit the state base address for the primary below, the
only change in here is that if the base state address of the primary is
the same as the one for the secondaries we won't actually emit the
state packet, but that should be fine. Maybe you thought I was removing
the line below?
>
> > */
> >genX(cmd_buffer_emit_state_base_address)(primary);
> > }
> > --
> > 2.14.1
>
>
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] anv/cmd_buffer: only emit state base address if the address changes
On June 29, 2018 03:11:00 Iago Toral Quiroga wrote:
---
src/intel/vulkan/anv_private.h | 5 +
src/intel/vulkan/genX_cmd_buffer.c | 12 +++-
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 510471da602..1a9ab7013f2 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -1989,6 +1989,11 @@ struct anv_cmd_state {
* is one of the states in render_pass_states.
*/
struct anv_state null_surface_state;
+
+ /**
+* Current state base address.
+*/
+ struct anv_address base_state_address;
};
struct anv_cmd_pool {
diff --git a/src/intel/vulkan/genX_cmd_buffer.c
b/src/intel/vulkan/genX_cmd_buffer.c
index 611311904e6..2847e0b30c9 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -67,6 +67,12 @@ genX(cmd_buffer_emit_state_base_address)(struct
anv_cmd_buffer *cmd_buffer)
{
struct anv_device *device = cmd_buffer->device;
+ struct anv_address new_base_address =
+ anv_cmd_buffer_surface_base_address(cmd_buffer);
+ if (new_base_address.bo == cmd_buffer->state.base_state_address.bo &&
+ new_base_address.offset == cmd_buffer->state.base_state_address.offset)
+ return;
+
/* If we are emitting a new state base address we probably need to re-emit
* binding tables.
*/
@@ -90,8 +96,7 @@ genX(cmd_buffer_emit_state_base_address)(struct
anv_cmd_buffer *cmd_buffer)
sba.GeneralStateMemoryObjectControlState = GENX(MOCS);
sba.GeneralStateBaseAddressModifyEnable = true;
- sba.SurfaceStateBaseAddress =
- anv_cmd_buffer_surface_base_address(cmd_buffer);
+ sba.SurfaceStateBaseAddress = new_base_address;
sba.SurfaceStateMemoryObjectControlState = GENX(MOCS);
sba.SurfaceStateBaseAddressModifyEnable = true;
@@ -1521,9 +1526,6 @@ genX(CmdExecuteCommands)(
/* Each of the secondary command buffers will use its own state base
* address. We need to re-emit state base address for the primary after
* all of the secondaries are done.
-*
-* TODO: Maybe we want to make this a dirty bit to avoid extra state base
-* address calls?
I don't think this is correct. When a secondary executes, we have to
reemit STATE_BASE_ADDRESS because the secondary used it's own and we need
to set it back for the primary. The comment above was saying that we can
probably avoid it if we have a bunch of ExecuteCommands calls back to back
or if the last thing in the batch is a call out to a secondary. As is, I
think this patch will cause problems in the case where the client uses a
secondary followed by rendering in the primary. Have I missed something?
*/
genX(cmd_buffer_emit_state_base_address)(primary);
}
--
2.14.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 4/4] anv/cmd_buffer: only emit state base address if the address changes
---
src/intel/vulkan/anv_private.h | 5 +
src/intel/vulkan/genX_cmd_buffer.c | 12 +++-
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 510471da602..1a9ab7013f2 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -1989,6 +1989,11 @@ struct anv_cmd_state {
* is one of the states in render_pass_states.
*/
struct anv_state null_surface_state;
+
+ /**
+* Current state base address.
+*/
+ struct anv_address base_state_address;
};
struct anv_cmd_pool {
diff --git a/src/intel/vulkan/genX_cmd_buffer.c
b/src/intel/vulkan/genX_cmd_buffer.c
index 611311904e6..2847e0b30c9 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -67,6 +67,12 @@ genX(cmd_buffer_emit_state_base_address)(struct
anv_cmd_buffer *cmd_buffer)
{
struct anv_device *device = cmd_buffer->device;
+ struct anv_address new_base_address =
+ anv_cmd_buffer_surface_base_address(cmd_buffer);
+ if (new_base_address.bo == cmd_buffer->state.base_state_address.bo &&
+ new_base_address.offset == cmd_buffer->state.base_state_address.offset)
+ return;
+
/* If we are emitting a new state base address we probably need to re-emit
* binding tables.
*/
@@ -90,8 +96,7 @@ genX(cmd_buffer_emit_state_base_address)(struct
anv_cmd_buffer *cmd_buffer)
sba.GeneralStateMemoryObjectControlState = GENX(MOCS);
sba.GeneralStateBaseAddressModifyEnable = true;
- sba.SurfaceStateBaseAddress =
- anv_cmd_buffer_surface_base_address(cmd_buffer);
+ sba.SurfaceStateBaseAddress = new_base_address;
sba.SurfaceStateMemoryObjectControlState = GENX(MOCS);
sba.SurfaceStateBaseAddressModifyEnable = true;
@@ -1521,9 +1526,6 @@ genX(CmdExecuteCommands)(
/* Each of the secondary command buffers will use its own state base
* address. We need to re-emit state base address for the primary after
* all of the secondaries are done.
-*
-* TODO: Maybe we want to make this a dirty bit to avoid extra state base
-* address calls?
*/
genX(cmd_buffer_emit_state_base_address)(primary);
}
--
2.14.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
