Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-13 Thread Christian König 

Me as well.

Christian.

Am 13.04.23 um 20:23 schrieb Marek Olšák:

I'm OK with that.

Marek

On Thu, Apr 13, 2023 at 12:56 PM Alex Deucher  
wrote:


On Thu, Apr 13, 2023 at 11:54 AM Christian König
 wrote:
>
> Am 13.04.23 um 14:26 schrieb Alex Deucher:
> > On Thu, Apr 13, 2023 at 7:32 AM Christian König
> >  wrote:
> >> Ok, then we have a problem.
> >>
> >> Alex what do you think?
> > If you program it to 0, FW skips the GDS backup I think so
UMD's can
> > decide whether they want to use it or not, depending on
whether they
> > use GDS.
>
> Yeah, but when Mesa isn't using it we have a hard way justifying to
> upstream that because it isn't tested at all.

Well, the interface would still get used, it's just that mesa would
likely only ever pass 0 for the virtual address.  It's just a
passthrough to the packet.  If we discover we need it at some point,
it would be nice to not have to add a new interface to add it.

Alex

>
> Christian.
>
> >
> > Alex
> >
> >
> >> Christian.
> >>
> >> Am 13.04.23 um 11:21 schrieb Marek Olšák:
> >>
> >> That's not why it was removed. It was removed because
userspace doesn't use GDS memory and gds_va is always going to be 0.
> >>
> >> Firmware shouldn't use it because using it would increase
preemption latency.
> >>
> >> Marek
> >>
> >> On Sun, Apr 9, 2023, 11:21 Christian König
 wrote:
> >>> We removed the GDS information because they were
unnecessary. The GDS size was already part of the device info
before we added the shadow info.
> >>>
> >>> But as far as I know the firmware needs valid VAs for all
three buffers or won't work correctly.
> >>>
> >>> Christian.
> >>>
> >>> Am 06.04.23 um 17:01 schrieb Marek Olšák:
> >>>
> >>> There is no GDS shadowing info in the device info uapi, so
userspace can't create any GDS buffer and thus can't have any GDS
va. It's a uapi issue, not what firmware wants to do.
> >>>
> >>> Marek
> >>>
> >>> On Thu, Apr 6, 2023 at 6:31 AM Christian König
 wrote:
>  That's what I thought as well, but Mitch/Hans insisted on that.
> 
>  We should probably double check internally.
> 
>  Christian.
> 
>  Am 06.04.23 um 11:43 schrieb Marek Olšák:
> 
>  GDS memory isn't used on gfx11. Only GDS OA is used.
> 
>  Marek
> 
>  On Thu, Apr 6, 2023 at 5:09 AM Christian König
 wrote:
> > Why that?
> >
> > This is the save buffer for GDS, not the old style GDS BOs.
> >
> > Christian.
> >
> > Am 06.04.23 um 09:36 schrieb Marek Olšák:
> >
> > gds_va is unnecessary.
> >
> > Marek
> >
> > On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher
 wrote:
> >> For GFX11, the UMD needs to allocate some shadow buffers
> >> to be used for preemption.  The UMD allocates the buffers
> >> and passes the GPU virtual address to the kernel since the
> >> kernel will program the packet that specified these
> >> addresses as part of its IB submission frame.
> >>
> >> v2: UMD passes shadow init to tell kernel when to initialize
> >>      the shadow
> >>
> >> Reviewed-by: Christian König 
> >> Signed-off-by: Alex Deucher 
> >> ---
> >>   include/uapi/drm/amdgpu_drm.h | 10 ++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/include/uapi/drm/amdgpu_drm.h
b/include/uapi/drm/amdgpu_drm.h
> >> index b6eb90df5d05..3d9474af6566 100644
> >> --- a/include/uapi/drm/amdgpu_drm.h
> >> +++ b/include/uapi/drm/amdgpu_drm.h
> >> @@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
> >>   #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
> >>   #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT    0x08
> >>   #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
> >> +#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a
> >>
> >>   struct drm_amdgpu_cs_chunk {
> >>          __u32  chunk_id;
> >> @@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
> >>          };
> >>   };
> >>
> >> +#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW 
       0x1
> >> +
> >> +struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
> >> +       __u64 shadow_va;
> >> +       __u64 csa_va;
> >> +       __u64 gds_va;
> >> +       __u64 flags;
> >> +};
> >> +
> >>   /*
> >>    *  Query h/w info: Flag that this is integrated
(a.h.a. fusion) GPU
> >>    *
> >> --
> >> 2.39.2
> >>
>

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-13 Thread Marek Olšák 
I'm OK with that.

Marek

On Thu, Apr 13, 2023 at 12:56 PM Alex Deucher  wrote:

> On Thu, Apr 13, 2023 at 11:54 AM Christian König
>  wrote:
> >
> > Am 13.04.23 um 14:26 schrieb Alex Deucher:
> > > On Thu, Apr 13, 2023 at 7:32 AM Christian König
> > >  wrote:
> > >> Ok, then we have a problem.
> > >>
> > >> Alex what do you think?
> > > If you program it to 0, FW skips the GDS backup I think so UMD's can
> > > decide whether they want to use it or not, depending on whether they
> > > use GDS.
> >
> > Yeah, but when Mesa isn't using it we have a hard way justifying to
> > upstream that because it isn't tested at all.
>
> Well, the interface would still get used, it's just that mesa would
> likely only ever pass 0 for the virtual address.  It's just a
> passthrough to the packet.  If we discover we need it at some point,
> it would be nice to not have to add a new interface to add it.
>
> Alex
>
> >
> > Christian.
> >
> > >
> > > Alex
> > >
> > >
> > >> Christian.
> > >>
> > >> Am 13.04.23 um 11:21 schrieb Marek Olšák:
> > >>
> > >> That's not why it was removed. It was removed because userspace
> doesn't use GDS memory and gds_va is always going to be 0.
> > >>
> > >> Firmware shouldn't use it because using it would increase preemption
> latency.
> > >>
> > >> Marek
> > >>
> > >> On Sun, Apr 9, 2023, 11:21 Christian König <
> ckoenig.leichtzumer...@gmail.com> wrote:
> > >>> We removed the GDS information because they were unnecessary. The
> GDS size was already part of the device info before we added the shadow
> info.
> > >>>
> > >>> But as far as I know the firmware needs valid VAs for all three
> buffers or won't work correctly.
> > >>>
> > >>> Christian.
> > >>>
> > >>> Am 06.04.23 um 17:01 schrieb Marek Olšák:
> > >>>
> > >>> There is no GDS shadowing info in the device info uapi, so userspace
> can't create any GDS buffer and thus can't have any GDS va. It's a uapi
> issue, not what firmware wants to do.
> > >>>
> > >>> Marek
> > >>>
> > >>> On Thu, Apr 6, 2023 at 6:31 AM Christian König <
> ckoenig.leichtzumer...@gmail.com> wrote:
> >  That's what I thought as well, but Mitch/Hans insisted on that.
> > 
> >  We should probably double check internally.
> > 
> >  Christian.
> > 
> >  Am 06.04.23 um 11:43 schrieb Marek Olšák:
> > 
> >  GDS memory isn't used on gfx11. Only GDS OA is used.
> > 
> >  Marek
> > 
> >  On Thu, Apr 6, 2023 at 5:09 AM Christian König <
> christian.koe...@amd.com> wrote:
> > > Why that?
> > >
> > > This is the save buffer for GDS, not the old style GDS BOs.
> > >
> > > Christian.
> > >
> > > Am 06.04.23 um 09:36 schrieb Marek Olšák:
> > >
> > > gds_va is unnecessary.
> > >
> > > Marek
> > >
> > > On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher <
> alexander.deuc...@amd.com> wrote:
> > >> For GFX11, the UMD needs to allocate some shadow buffers
> > >> to be used for preemption.  The UMD allocates the buffers
> > >> and passes the GPU virtual address to the kernel since the
> > >> kernel will program the packet that specified these
> > >> addresses as part of its IB submission frame.
> > >>
> > >> v2: UMD passes shadow init to tell kernel when to initialize
> > >>  the shadow
> > >>
> > >> Reviewed-by: Christian König 
> > >> Signed-off-by: Alex Deucher 
> > >> ---
> > >>   include/uapi/drm/amdgpu_drm.h | 10 ++
> > >>   1 file changed, 10 insertions(+)
> > >>
> > >> diff --git a/include/uapi/drm/amdgpu_drm.h
> b/include/uapi/drm/amdgpu_drm.h
> > >> index b6eb90df5d05..3d9474af6566 100644
> > >> --- a/include/uapi/drm/amdgpu_drm.h
> > >> +++ b/include/uapi/drm/amdgpu_drm.h
> > >> @@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
> > >>   #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
> > >>   #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT0x08
> > >>   #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
> > >> +#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a
> > >>
> > >>   struct drm_amdgpu_cs_chunk {
> > >>  __u32   chunk_id;
> > >> @@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
> > >>  };
> > >>   };
> > >>
> > >> +#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW
>  0x1
> > >> +
> > >> +struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
> > >> +   __u64 shadow_va;
> > >> +   __u64 csa_va;
> > >> +   __u64 gds_va;
> > >> +   __u64 flags;
> > >> +};
> > >> +
> > >>   /*
> > >>*  Query h/w info: Flag that this is integrated (a.h.a.
> fusion) GPU
> > >>*
> > >> --
> > >> 2.39.2
> > >>
> >
>

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-13 Thread Alex Deucher 
On Thu, Apr 13, 2023 at 11:54 AM Christian König
 wrote:
>
> Am 13.04.23 um 14:26 schrieb Alex Deucher:
> > On Thu, Apr 13, 2023 at 7:32 AM Christian König
> >  wrote:
> >> Ok, then we have a problem.
> >>
> >> Alex what do you think?
> > If you program it to 0, FW skips the GDS backup I think so UMD's can
> > decide whether they want to use it or not, depending on whether they
> > use GDS.
>
> Yeah, but when Mesa isn't using it we have a hard way justifying to
> upstream that because it isn't tested at all.

Well, the interface would still get used, it's just that mesa would
likely only ever pass 0 for the virtual address.  It's just a
passthrough to the packet.  If we discover we need it at some point,
it would be nice to not have to add a new interface to add it.

Alex

>
> Christian.
>
> >
> > Alex
> >
> >
> >> Christian.
> >>
> >> Am 13.04.23 um 11:21 schrieb Marek Olšák:
> >>
> >> That's not why it was removed. It was removed because userspace doesn't 
> >> use GDS memory and gds_va is always going to be 0.
> >>
> >> Firmware shouldn't use it because using it would increase preemption 
> >> latency.
> >>
> >> Marek
> >>
> >> On Sun, Apr 9, 2023, 11:21 Christian König 
> >>  wrote:
> >>> We removed the GDS information because they were unnecessary. The GDS 
> >>> size was already part of the device info before we added the shadow info.
> >>>
> >>> But as far as I know the firmware needs valid VAs for all three buffers 
> >>> or won't work correctly.
> >>>
> >>> Christian.
> >>>
> >>> Am 06.04.23 um 17:01 schrieb Marek Olšák:
> >>>
> >>> There is no GDS shadowing info in the device info uapi, so userspace 
> >>> can't create any GDS buffer and thus can't have any GDS va. It's a uapi 
> >>> issue, not what firmware wants to do.
> >>>
> >>> Marek
> >>>
> >>> On Thu, Apr 6, 2023 at 6:31 AM Christian König 
> >>>  wrote:
>  That's what I thought as well, but Mitch/Hans insisted on that.
> 
>  We should probably double check internally.
> 
>  Christian.
> 
>  Am 06.04.23 um 11:43 schrieb Marek Olšák:
> 
>  GDS memory isn't used on gfx11. Only GDS OA is used.
> 
>  Marek
> 
>  On Thu, Apr 6, 2023 at 5:09 AM Christian König 
>   wrote:
> > Why that?
> >
> > This is the save buffer for GDS, not the old style GDS BOs.
> >
> > Christian.
> >
> > Am 06.04.23 um 09:36 schrieb Marek Olšák:
> >
> > gds_va is unnecessary.
> >
> > Marek
> >
> > On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher 
> >  wrote:
> >> For GFX11, the UMD needs to allocate some shadow buffers
> >> to be used for preemption.  The UMD allocates the buffers
> >> and passes the GPU virtual address to the kernel since the
> >> kernel will program the packet that specified these
> >> addresses as part of its IB submission frame.
> >>
> >> v2: UMD passes shadow init to tell kernel when to initialize
> >>  the shadow
> >>
> >> Reviewed-by: Christian König 
> >> Signed-off-by: Alex Deucher 
> >> ---
> >>   include/uapi/drm/amdgpu_drm.h | 10 ++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/include/uapi/drm/amdgpu_drm.h 
> >> b/include/uapi/drm/amdgpu_drm.h
> >> index b6eb90df5d05..3d9474af6566 100644
> >> --- a/include/uapi/drm/amdgpu_drm.h
> >> +++ b/include/uapi/drm/amdgpu_drm.h
> >> @@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
> >>   #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
> >>   #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT0x08
> >>   #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
> >> +#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a
> >>
> >>   struct drm_amdgpu_cs_chunk {
> >>  __u32   chunk_id;
> >> @@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
> >>  };
> >>   };
> >>
> >> +#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW 0x1
> >> +
> >> +struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
> >> +   __u64 shadow_va;
> >> +   __u64 csa_va;
> >> +   __u64 gds_va;
> >> +   __u64 flags;
> >> +};
> >> +
> >>   /*
> >>*  Query h/w info: Flag that this is integrated (a.h.a. fusion) GPU
> >>*
> >> --
> >> 2.39.2
> >>
>

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-13 Thread Christian König 

Am 13.04.23 um 14:26 schrieb Alex Deucher:

On Thu, Apr 13, 2023 at 7:32 AM Christian König
 wrote:

Ok, then we have a problem.

Alex what do you think?

If you program it to 0, FW skips the GDS backup I think so UMD's can
decide whether they want to use it or not, depending on whether they
use GDS.


Yeah, but when Mesa isn't using it we have a hard way justifying to 
upstream that because it isn't tested at all.


Christian.



Alex



Christian.

Am 13.04.23 um 11:21 schrieb Marek Olšák:

That's not why it was removed. It was removed because userspace doesn't use GDS 
memory and gds_va is always going to be 0.

Firmware shouldn't use it because using it would increase preemption latency.

Marek

On Sun, Apr 9, 2023, 11:21 Christian König  
wrote:

We removed the GDS information because they were unnecessary. The GDS size was 
already part of the device info before we added the shadow info.

But as far as I know the firmware needs valid VAs for all three buffers or 
won't work correctly.

Christian.

Am 06.04.23 um 17:01 schrieb Marek Olšák:

There is no GDS shadowing info in the device info uapi, so userspace can't 
create any GDS buffer and thus can't have any GDS va. It's a uapi issue, not 
what firmware wants to do.

Marek

On Thu, Apr 6, 2023 at 6:31 AM Christian König 
 wrote:

That's what I thought as well, but Mitch/Hans insisted on that.

We should probably double check internally.

Christian.

Am 06.04.23 um 11:43 schrieb Marek Olšák:

GDS memory isn't used on gfx11. Only GDS OA is used.

Marek

On Thu, Apr 6, 2023 at 5:09 AM Christian König  wrote:

Why that?

This is the save buffer for GDS, not the old style GDS BOs.

Christian.

Am 06.04.23 um 09:36 schrieb Marek Olšák:

gds_va is unnecessary.

Marek

On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher  wrote:

For GFX11, the UMD needs to allocate some shadow buffers
to be used for preemption.  The UMD allocates the buffers
and passes the GPU virtual address to the kernel since the
kernel will program the packet that specified these
addresses as part of its IB submission frame.

v2: UMD passes shadow init to tell kernel when to initialize
 the shadow

Reviewed-by: Christian König 
Signed-off-by: Alex Deucher 
---
  include/uapi/drm/amdgpu_drm.h | 10 ++
  1 file changed, 10 insertions(+)

diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index b6eb90df5d05..3d9474af6566 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
  #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT0x08
  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
+#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a

  struct drm_amdgpu_cs_chunk {
 __u32   chunk_id;
@@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
 };
  };

+#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW 0x1
+
+struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
+   __u64 shadow_va;
+   __u64 csa_va;
+   __u64 gds_va;
+   __u64 flags;
+};
+
  /*
   *  Query h/w info: Flag that this is integrated (a.h.a. fusion) GPU
   *
--
2.39.2

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-13 Thread Alex Deucher 
On Thu, Apr 13, 2023 at 7:32 AM Christian König
 wrote:
>
> Ok, then we have a problem.
>
> Alex what do you think?

If you program it to 0, FW skips the GDS backup I think so UMD's can
decide whether they want to use it or not, depending on whether they
use GDS.

Alex


>
> Christian.
>
> Am 13.04.23 um 11:21 schrieb Marek Olšák:
>
> That's not why it was removed. It was removed because userspace doesn't use 
> GDS memory and gds_va is always going to be 0.
>
> Firmware shouldn't use it because using it would increase preemption latency.
>
> Marek
>
> On Sun, Apr 9, 2023, 11:21 Christian König  
> wrote:
>>
>> We removed the GDS information because they were unnecessary. The GDS size 
>> was already part of the device info before we added the shadow info.
>>
>> But as far as I know the firmware needs valid VAs for all three buffers or 
>> won't work correctly.
>>
>> Christian.
>>
>> Am 06.04.23 um 17:01 schrieb Marek Olšák:
>>
>> There is no GDS shadowing info in the device info uapi, so userspace can't 
>> create any GDS buffer and thus can't have any GDS va. It's a uapi issue, not 
>> what firmware wants to do.
>>
>> Marek
>>
>> On Thu, Apr 6, 2023 at 6:31 AM Christian König 
>>  wrote:
>>>
>>> That's what I thought as well, but Mitch/Hans insisted on that.
>>>
>>> We should probably double check internally.
>>>
>>> Christian.
>>>
>>> Am 06.04.23 um 11:43 schrieb Marek Olšák:
>>>
>>> GDS memory isn't used on gfx11. Only GDS OA is used.
>>>
>>> Marek
>>>
>>> On Thu, Apr 6, 2023 at 5:09 AM Christian König  
>>> wrote:

 Why that?

 This is the save buffer for GDS, not the old style GDS BOs.

 Christian.

 Am 06.04.23 um 09:36 schrieb Marek Olšák:

 gds_va is unnecessary.

 Marek

 On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher  
 wrote:
>
> For GFX11, the UMD needs to allocate some shadow buffers
> to be used for preemption.  The UMD allocates the buffers
> and passes the GPU virtual address to the kernel since the
> kernel will program the packet that specified these
> addresses as part of its IB submission frame.
>
> v2: UMD passes shadow init to tell kernel when to initialize
> the shadow
>
> Reviewed-by: Christian König 
> Signed-off-by: Alex Deucher 
> ---
>  include/uapi/drm/amdgpu_drm.h | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index b6eb90df5d05..3d9474af6566 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
>  #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
>  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT0x08
>  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
> +#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a
>
>  struct drm_amdgpu_cs_chunk {
> __u32   chunk_id;
> @@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
> };
>  };
>
> +#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW 0x1
> +
> +struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
> +   __u64 shadow_va;
> +   __u64 csa_va;
> +   __u64 gds_va;
> +   __u64 flags;
> +};
> +
>  /*
>   *  Query h/w info: Flag that this is integrated (a.h.a. fusion) GPU
>   *
> --
> 2.39.2
>

>>>
>>
>

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-13 Thread Christian König 

Ok, then we have a problem.

Alex what do you think?

Christian.

Am 13.04.23 um 11:21 schrieb Marek Olšák:
That's not why it was removed. It was removed because userspace 
doesn't use GDS memory and gds_va is always going to be 0.


Firmware shouldn't use it because using it would increase preemption 
latency.


Marek

On Sun, Apr 9, 2023, 11:21 Christian König 
 wrote:


We removed the GDS information because they were unnecessary. The
GDS size was already part of the device info before we added the
shadow info.

But as far as I know the firmware needs valid VAs for all three
buffers or won't work correctly.

Christian.

Am 06.04.23 um 17:01 schrieb Marek Olšák:

There is no GDS shadowing info in the device info uapi, so
userspace can't create any GDS buffer and thus can't have any GDS
va. It's a uapi issue, not what firmware wants to do.

Marek

On Thu, Apr 6, 2023 at 6:31 AM Christian König
 wrote:

That's what I thought as well, but Mitch/Hans insisted on that.

We should probably double check internally.

Christian.

Am 06.04.23 um 11:43 schrieb Marek Olšák:

GDS memory isn't used on gfx11. Only GDS OA is used.

Marek

On Thu, Apr 6, 2023 at 5:09 AM Christian König
 wrote:

Why that?

This is the save buffer for GDS, not the old style GDS BOs.

Christian.

Am 06.04.23 um 09:36 schrieb Marek Olšák:

gds_va is unnecessary.

Marek

On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher
 wrote:

For GFX11, the UMD needs to allocate some shadow
buffers
to be used for preemption.  The UMD allocates the
buffers
and passes the GPU virtual address to the kernel
since the
kernel will program the packet that specified these
addresses as part of its IB submission frame.

v2: UMD passes shadow init to tell kernel when to
initialize
    the shadow

Reviewed-by: Christian König 
Signed-off-by: Alex Deucher 
---
 include/uapi/drm/amdgpu_drm.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/include/uapi/drm/amdgpu_drm.h
b/include/uapi/drm/amdgpu_drm.h
index b6eb90df5d05..3d9474af6566 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
 #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
 #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT   0x08
 #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL 0x09
+#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW  0x0a

 struct drm_amdgpu_cs_chunk {
        __u32           chunk_id;
@@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
        };
 };

+#define
AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW    
   0x1
+
+struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
+       __u64 shadow_va;
+       __u64 csa_va;
+       __u64 gds_va;
+       __u64 flags;
+};
+
 /*
  *  Query h/w info: Flag that this is integrated
(a.h.a. fusion) GPU
  *
-- 
2.39.2

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-13 Thread Marek Olšák 
That's not why it was removed. It was removed because userspace doesn't use
GDS memory and gds_va is always going to be 0.

Firmware shouldn't use it because using it would increase preemption
latency.

Marek

On Sun, Apr 9, 2023, 11:21 Christian König 
wrote:

> We removed the GDS information because they were unnecessary. The GDS size
> was already part of the device info before we added the shadow info.
>
> But as far as I know the firmware needs valid VAs for all three buffers or
> won't work correctly.
>
> Christian.
>
> Am 06.04.23 um 17:01 schrieb Marek Olšák:
>
> There is no GDS shadowing info in the device info uapi, so userspace can't
> create any GDS buffer and thus can't have any GDS va. It's a uapi issue,
> not what firmware wants to do.
>
> Marek
>
> On Thu, Apr 6, 2023 at 6:31 AM Christian König <
> ckoenig.leichtzumer...@gmail.com> wrote:
>
>> That's what I thought as well, but Mitch/Hans insisted on that.
>>
>> We should probably double check internally.
>>
>> Christian.
>>
>> Am 06.04.23 um 11:43 schrieb Marek Olšák:
>>
>> GDS memory isn't used on gfx11. Only GDS OA is used.
>>
>> Marek
>>
>> On Thu, Apr 6, 2023 at 5:09 AM Christian König 
>> wrote:
>>
>>> Why that?
>>>
>>> This is the save buffer for GDS, not the old style GDS BOs.
>>>
>>> Christian.
>>>
>>> Am 06.04.23 um 09:36 schrieb Marek Olšák:
>>>
>>> gds_va is unnecessary.
>>>
>>> Marek
>>>
>>> On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher 
>>> wrote:
>>>
 For GFX11, the UMD needs to allocate some shadow buffers
 to be used for preemption.  The UMD allocates the buffers
 and passes the GPU virtual address to the kernel since the
 kernel will program the packet that specified these
 addresses as part of its IB submission frame.

 v2: UMD passes shadow init to tell kernel when to initialize
 the shadow

 Reviewed-by: Christian König 
 Signed-off-by: Alex Deucher 
 ---
  include/uapi/drm/amdgpu_drm.h | 10 ++
  1 file changed, 10 insertions(+)

 diff --git a/include/uapi/drm/amdgpu_drm.h
 b/include/uapi/drm/amdgpu_drm.h
 index b6eb90df5d05..3d9474af6566 100644
 --- a/include/uapi/drm/amdgpu_drm.h
 +++ b/include/uapi/drm/amdgpu_drm.h
 @@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
  #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT0x08
  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
 +#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a

  struct drm_amdgpu_cs_chunk {
 __u32   chunk_id;
 @@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
 };
  };

 +#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW 0x1
 +
 +struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
 +   __u64 shadow_va;
 +   __u64 csa_va;
 +   __u64 gds_va;
 +   __u64 flags;
 +};
 +
  /*
   *  Query h/w info: Flag that this is integrated (a.h.a. fusion) GPU
   *
 --
 2.39.2


>>>
>>
>

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-09 Thread Christian König 
We removed the GDS information because they were unnecessary. The GDS 
size was already part of the device info before we added the shadow info.


But as far as I know the firmware needs valid VAs for all three buffers 
or won't work correctly.


Christian.

Am 06.04.23 um 17:01 schrieb Marek Olšák:
There is no GDS shadowing info in the device info uapi, so userspace 
can't create any GDS buffer and thus can't have any GDS va. It's a 
uapi issue, not what firmware wants to do.


Marek

On Thu, Apr 6, 2023 at 6:31 AM Christian König 
 wrote:


That's what I thought as well, but Mitch/Hans insisted on that.

We should probably double check internally.

Christian.

Am 06.04.23 um 11:43 schrieb Marek Olšák:

GDS memory isn't used on gfx11. Only GDS OA is used.

Marek

On Thu, Apr 6, 2023 at 5:09 AM Christian König
 wrote:

Why that?

This is the save buffer for GDS, not the old style GDS BOs.

Christian.

Am 06.04.23 um 09:36 schrieb Marek Olšák:

gds_va is unnecessary.

Marek

On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher
 wrote:

For GFX11, the UMD needs to allocate some shadow buffers
to be used for preemption.  The UMD allocates the buffers
and passes the GPU virtual address to the kernel since the
kernel will program the packet that specified these
addresses as part of its IB submission frame.

v2: UMD passes shadow init to tell kernel when to initialize
    the shadow

Reviewed-by: Christian König 
Signed-off-by: Alex Deucher 
---
 include/uapi/drm/amdgpu_drm.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/include/uapi/drm/amdgpu_drm.h
b/include/uapi/drm/amdgpu_drm.h
index b6eb90df5d05..3d9474af6566 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
 #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
 #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT    0x08
 #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
+#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a

 struct drm_amdgpu_cs_chunk {
        __u32           chunk_id;
@@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
        };
 };

+#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW
       0x1
+
+struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
+       __u64 shadow_va;
+       __u64 csa_va;
+       __u64 gds_va;
+       __u64 flags;
+};
+
 /*
  *  Query h/w info: Flag that this is integrated
(a.h.a. fusion) GPU
  *
-- 
2.39.2

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-06 Thread Marek Olšák 
There is no GDS shadowing info in the device info uapi, so userspace can't
create any GDS buffer and thus can't have any GDS va. It's a uapi issue,
not what firmware wants to do.

Marek

On Thu, Apr 6, 2023 at 6:31 AM Christian König <
ckoenig.leichtzumer...@gmail.com> wrote:

> That's what I thought as well, but Mitch/Hans insisted on that.
>
> We should probably double check internally.
>
> Christian.
>
> Am 06.04.23 um 11:43 schrieb Marek Olšák:
>
> GDS memory isn't used on gfx11. Only GDS OA is used.
>
> Marek
>
> On Thu, Apr 6, 2023 at 5:09 AM Christian König 
> wrote:
>
>> Why that?
>>
>> This is the save buffer for GDS, not the old style GDS BOs.
>>
>> Christian.
>>
>> Am 06.04.23 um 09:36 schrieb Marek Olšák:
>>
>> gds_va is unnecessary.
>>
>> Marek
>>
>> On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher 
>> wrote:
>>
>>> For GFX11, the UMD needs to allocate some shadow buffers
>>> to be used for preemption.  The UMD allocates the buffers
>>> and passes the GPU virtual address to the kernel since the
>>> kernel will program the packet that specified these
>>> addresses as part of its IB submission frame.
>>>
>>> v2: UMD passes shadow init to tell kernel when to initialize
>>> the shadow
>>>
>>> Reviewed-by: Christian König 
>>> Signed-off-by: Alex Deucher 
>>> ---
>>>  include/uapi/drm/amdgpu_drm.h | 10 ++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/include/uapi/drm/amdgpu_drm.h
>>> b/include/uapi/drm/amdgpu_drm.h
>>> index b6eb90df5d05..3d9474af6566 100644
>>> --- a/include/uapi/drm/amdgpu_drm.h
>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>> @@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
>>>  #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
>>>  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT0x08
>>>  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
>>> +#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a
>>>
>>>  struct drm_amdgpu_cs_chunk {
>>> __u32   chunk_id;
>>> @@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
>>> };
>>>  };
>>>
>>> +#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW 0x1
>>> +
>>> +struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
>>> +   __u64 shadow_va;
>>> +   __u64 csa_va;
>>> +   __u64 gds_va;
>>> +   __u64 flags;
>>> +};
>>> +
>>>  /*
>>>   *  Query h/w info: Flag that this is integrated (a.h.a. fusion) GPU
>>>   *
>>> --
>>> 2.39.2
>>>
>>>
>>
>

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-06 Thread Christian König 

That's what I thought as well, but Mitch/Hans insisted on that.

We should probably double check internally.

Christian.

Am 06.04.23 um 11:43 schrieb Marek Olšák:

GDS memory isn't used on gfx11. Only GDS OA is used.

Marek

On Thu, Apr 6, 2023 at 5:09 AM Christian König 
 wrote:


Why that?

This is the save buffer for GDS, not the old style GDS BOs.

Christian.

Am 06.04.23 um 09:36 schrieb Marek Olšák:

gds_va is unnecessary.

Marek

On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher
 wrote:

For GFX11, the UMD needs to allocate some shadow buffers
to be used for preemption.  The UMD allocates the buffers
and passes the GPU virtual address to the kernel since the
kernel will program the packet that specified these
addresses as part of its IB submission frame.

v2: UMD passes shadow init to tell kernel when to initialize
    the shadow

Reviewed-by: Christian König 
Signed-off-by: Alex Deucher 
---
 include/uapi/drm/amdgpu_drm.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/include/uapi/drm/amdgpu_drm.h
b/include/uapi/drm/amdgpu_drm.h
index b6eb90df5d05..3d9474af6566 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
 #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
 #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT    0x08
 #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
+#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a

 struct drm_amdgpu_cs_chunk {
        __u32           chunk_id;
@@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
        };
 };

+#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW  0x1
+
+struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
+       __u64 shadow_va;
+       __u64 csa_va;
+       __u64 gds_va;
+       __u64 flags;
+};
+
 /*
  *  Query h/w info: Flag that this is integrated (a.h.a.
fusion) GPU
  *
-- 
2.39.2

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-06 Thread Marek Olšák 
GDS memory isn't used on gfx11. Only GDS OA is used.

Marek

On Thu, Apr 6, 2023 at 5:09 AM Christian König 
wrote:

> Why that?
>
> This is the save buffer for GDS, not the old style GDS BOs.
>
> Christian.
>
> Am 06.04.23 um 09:36 schrieb Marek Olšák:
>
> gds_va is unnecessary.
>
> Marek
>
> On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher 
> wrote:
>
>> For GFX11, the UMD needs to allocate some shadow buffers
>> to be used for preemption.  The UMD allocates the buffers
>> and passes the GPU virtual address to the kernel since the
>> kernel will program the packet that specified these
>> addresses as part of its IB submission frame.
>>
>> v2: UMD passes shadow init to tell kernel when to initialize
>> the shadow
>>
>> Reviewed-by: Christian König 
>> Signed-off-by: Alex Deucher 
>> ---
>>  include/uapi/drm/amdgpu_drm.h | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>> index b6eb90df5d05..3d9474af6566 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
>>  #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
>>  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT0x08
>>  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
>> +#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a
>>
>>  struct drm_amdgpu_cs_chunk {
>> __u32   chunk_id;
>> @@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
>> };
>>  };
>>
>> +#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW 0x1
>> +
>> +struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
>> +   __u64 shadow_va;
>> +   __u64 csa_va;
>> +   __u64 gds_va;
>> +   __u64 flags;
>> +};
>> +
>>  /*
>>   *  Query h/w info: Flag that this is integrated (a.h.a. fusion) GPU
>>   *
>> --
>> 2.39.2
>>
>>
>

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-06 Thread Christian König 

Why that?

This is the save buffer for GDS, not the old style GDS BOs.

Christian.

Am 06.04.23 um 09:36 schrieb Marek Olšák:

gds_va is unnecessary.

Marek

On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher 
 wrote:


For GFX11, the UMD needs to allocate some shadow buffers
to be used for preemption.  The UMD allocates the buffers
and passes the GPU virtual address to the kernel since the
kernel will program the packet that specified these
addresses as part of its IB submission frame.

v2: UMD passes shadow init to tell kernel when to initialize
    the shadow

Reviewed-by: Christian König 
Signed-off-by: Alex Deucher 
---
 include/uapi/drm/amdgpu_drm.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/include/uapi/drm/amdgpu_drm.h
b/include/uapi/drm/amdgpu_drm.h
index b6eb90df5d05..3d9474af6566 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
 #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
 #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT    0x08
 #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
+#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a

 struct drm_amdgpu_cs_chunk {
        __u32           chunk_id;
@@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
        };
 };

+#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW    0x1
+
+struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
+       __u64 shadow_va;
+       __u64 csa_va;
+       __u64 gds_va;
+       __u64 flags;
+};
+
 /*
  *  Query h/w info: Flag that this is integrated (a.h.a. fusion) GPU
  *
-- 
2.39.2

Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers

2023-04-06 Thread Marek Olšák 
gds_va is unnecessary.

Marek

On Thu, Mar 30, 2023 at 3:18 PM Alex Deucher 
wrote:

> For GFX11, the UMD needs to allocate some shadow buffers
> to be used for preemption.  The UMD allocates the buffers
> and passes the GPU virtual address to the kernel since the
> kernel will program the packet that specified these
> addresses as part of its IB submission frame.
>
> v2: UMD passes shadow init to tell kernel when to initialize
> the shadow
>
> Reviewed-by: Christian König 
> Signed-off-by: Alex Deucher 
> ---
>  include/uapi/drm/amdgpu_drm.h | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index b6eb90df5d05..3d9474af6566 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va {
>  #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07
>  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT0x08
>  #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x09
> +#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW   0x0a
>
>  struct drm_amdgpu_cs_chunk {
> __u32   chunk_id;
> @@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data {
> };
>  };
>
> +#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW 0x1
> +
> +struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
> +   __u64 shadow_va;
> +   __u64 csa_va;
> +   __u64 gds_va;
> +   __u64 flags;
> +};
> +
>  /*
>   *  Query h/w info: Flag that this is integrated (a.h.a. fusion) GPU
>   *
> --
> 2.39.2
>
>