Re: [PATCH -next] drm/amdgpu: remove set but not used variables 'ret'

[Julia Lawall]

On Sat, 22 Jun 2019, Mao Wenan wrote:

> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’:
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ set 
> but not used [-Wunused-but-set-variable]
>   int ret = 0;
>   ^
>
> It is never used since introduction in 9c7c85f7ea1f ("drm/amdgpu: add pmu 
> counters")
>
> Signed-off-by: Mao Wenan 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
> index 0e6dba9..0bf4dd9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
> @@ -246,12 +246,10 @@ static int init_pmu_by_type(struct amdgpu_device *adev,
>  /* init amdgpu_pmu */
>  int amdgpu_pmu_init(struct amdgpu_device *adev)
>  {
> - int ret = 0;
> -
>   switch (adev->asic_type) {
>   case CHIP_VEGA20:
>   /* init df */
> - ret = init_pmu_by_type(adev, df_v3_6_attr_groups,
> + init_pmu_by_type(adev, df_v3_6_attr_groups,
>  "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,
>  DF_V3_6_MAX_COUNTERS);

Maybe it would be better to use ret?

If knowing whether the call has failed is really useless, then maybe the
return type should be void?

julia


>
> --
> 2.7.4
>
>

[PATCH -next] drm/amdgpu: remove set but not used variables 'ret'

[Mao Wenan]

Fixes gcc '-Wunused-but-set-variable' warning:

drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c: In function ‘amdgpu_pmu_init’:
drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:249:6: warning: variable ‘ret’ set but 
not used [-Wunused-but-set-variable]
  int ret = 0;
  ^

It is never used since introduction in 9c7c85f7ea1f ("drm/amdgpu: add pmu 
counters")

Signed-off-by: Mao Wenan 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
index 0e6dba9..0bf4dd9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c
@@ -246,12 +246,10 @@ static int init_pmu_by_type(struct amdgpu_device *adev,
 /* init amdgpu_pmu */
 int amdgpu_pmu_init(struct amdgpu_device *adev)
 {
-   int ret = 0;
-
switch (adev->asic_type) {
case CHIP_VEGA20:
/* init df */
-   ret = init_pmu_by_type(adev, df_v3_6_attr_groups,
+   init_pmu_by_type(adev, df_v3_6_attr_groups,
   "DF", "amdgpu_df", PERF_TYPE_AMDGPU_DF,
   DF_V3_6_MAX_COUNTERS);
 
-- 
2.7.4

[PATCH 1/1] drm/amdkfd: Consistently apply noretry setting

[Kuehling, Felix]

Apply the same setting to SH_MEM_CONFIG and VM_CONTEXT1_CNTL. This
makes the noretry param no longer KFD-specific. On GFX10 I'm not
changing SH_MEM_CONFIG in this commit because GFX10 has different
retry behaviour in the SQ and I don't have a way to test it at the
moment.

Suggested-by: Christian König 
CC: Philip Yang 
Signed-off-by: Felix Kuehling 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h  |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 16 +---
 drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c|  4 
 drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c |  3 ++-
 drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c |  3 ++-
 drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  |  3 ++-
 drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c  |  3 ++-
 .../drm/amd/amdkfd/kfd_device_queue_manager_v9.c |  2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h|  2 +-
 9 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 9b1efdf94bdf..05875279c09e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -164,6 +164,7 @@ extern int amdgpu_async_gfx_ring;
 extern int amdgpu_mcbp;
 extern int amdgpu_discovery;
 extern int amdgpu_mes;
+extern int amdgpu_noretry;
 
 #ifdef CONFIG_DRM_AMDGPU_SI
 extern int amdgpu_si_support;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 7cf6ab07b113..0d578d95be93 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -140,6 +140,7 @@ int amdgpu_async_gfx_ring = 1;
 int amdgpu_mcbp = 0;
 int amdgpu_discovery = 0;
 int amdgpu_mes = 0;
+int amdgpu_noretry;
 
 struct amdgpu_mgpu_info mgpu_info = {
.mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
@@ -591,6 +592,10 @@ MODULE_PARM_DESC(mes,
"Enable Micro Engine Scheduler (0 = disabled (default), 1 = enabled)");
 module_param_named(mes, amdgpu_mes, int, 0444);
 
+MODULE_PARM_DESC(noretry,
+   "Disable retry faults (0 = retry enabled (default), 1 = retry 
disabled)");
+module_param_named(noretry, amdgpu_noretry, int, 0644);
+
 #ifdef CONFIG_HSA_AMD
 /**
  * DOC: sched_policy (int)
@@ -666,17 +671,6 @@ module_param(ignore_crat, int, 0444);
 MODULE_PARM_DESC(ignore_crat,
"Ignore CRAT table during KFD initialization (0 = use CRAT (default), 1 
= ignore CRAT)");
 
-/**
- * DOC: noretry (int)
- * This parameter sets sh_mem_config.retry_disable. Default value, 0, enables 
retry.
- * Setting 1 disables retry.
- * Retry is needed for recoverable page faults.
- */
-int noretry;
-module_param(noretry, int, 0644);
-MODULE_PARM_DESC(noretry,
-   "Set sh_mem_config.retry_disable on Vega10 (0 = retry enabled 
(default), 1 = retry disabled)");
-
 /**
  * DOC: halt_if_hws_hang (int)
  * Halt if HWS hang is detected. Default value, 0, disables the halt on hang.
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index e0f3014e76ea..c4e715170bfe 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -1938,11 +1938,15 @@ static void gfx_v9_0_constants_init(struct 
amdgpu_device *adev)
if (i == 0) {
tmp = REG_SET_FIELD(0, SH_MEM_CONFIG, ALIGNMENT_MODE,
SH_MEM_ALIGNMENT_MODE_UNALIGNED);
+   tmp = REG_SET_FIELD(tmp, SH_MEM_CONFIG, RETRY_DISABLE,
+   !!amdgpu_noretry);
WREG32_SOC15_RLC(GC, 0, mmSH_MEM_CONFIG, tmp);
WREG32_SOC15_RLC(GC, 0, mmSH_MEM_BASES, 0);
} else {
tmp = REG_SET_FIELD(0, SH_MEM_CONFIG, ALIGNMENT_MODE,
SH_MEM_ALIGNMENT_MODE_UNALIGNED);
+   tmp = REG_SET_FIELD(tmp, SH_MEM_CONFIG, RETRY_DISABLE,
+   !!amdgpu_noretry);
WREG32_SOC15_RLC(GC, 0, mmSH_MEM_CONFIG, tmp);
tmp = REG_SET_FIELD(0, SH_MEM_BASES, PRIVATE_BASE,
(adev->gmc.private_aperture_start >> 48));
diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
index 9f0f189fc111..15986748f59f 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
@@ -236,7 +236,8 @@ static void gfxhub_v1_0_setup_vmid_config(struct 
amdgpu_device *adev)
block_size);
/* Send no-retry XNACK on fault to suppress VM fault storm. */
tmp = REG_SET_FIELD(tmp, VM_CONTEXT1_CNTL,
-   RETRY_PERMISSION_OR_INVALID_PAGE_FAULT, 1);
+   RETRY_PERMISSION_OR_INVALID_PAGE_FAULT,
+   !amdgpu_noretry);
WR

Re: [PATCH 1/2] drm/amd/amdgpu: Tabs instead of spaces in gfx_v10_0.c

[Alex Deucher]

On Mon, Jun 17, 2019 at 5:42 PM Ernst Sjöstrand  wrote:
>
> Done automatically with unexpand.
>
> Signed-off-by: Ernst Sjöstrand 

Thanks for the patches.  I've gone ahead and squashed them into the
original patches for upstream.

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 158 -
>  1 file changed, 79 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index 0090cba2d24d..d04f95ec4471 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -114,7 +114,7 @@ static void gfx_v10_0_set_irq_funcs(struct amdgpu_device 
> *adev);
>  static void gfx_v10_0_set_gds_init(struct amdgpu_device *adev);
>  static void gfx_v10_0_set_rlc_funcs(struct amdgpu_device *adev);
>  static int gfx_v10_0_get_cu_info(struct amdgpu_device *adev,
> - struct amdgpu_cu_info *cu_info);
> +struct amdgpu_cu_info *cu_info);
>  static uint64_t gfx_v10_0_get_gpu_clock_counter(struct amdgpu_device *adev);
>  static void gfx_v10_0_select_se_sh(struct amdgpu_device *adev, u32 se_num,
>u32 sh_num, u32 instance);
> @@ -345,63 +345,63 @@ static int gfx_v10_0_ring_test_ring(struct amdgpu_ring 
> *ring)
>
>  static int gfx_v10_0_ring_test_ib(struct amdgpu_ring *ring, long timeout)
>  {
> -struct amdgpu_device *adev = ring->adev;
> -struct amdgpu_ib ib;
> -struct dma_fence *f = NULL;
> -uint32_t scratch;
> -uint32_t tmp = 0;
> -long r;
> -
> -r = amdgpu_gfx_scratch_get(adev, &scratch);
> -if (r) {
> -DRM_ERROR("amdgpu: failed to get scratch reg (%ld).\n", r);
> -return r;
> -}
> -
> -WREG32(scratch, 0xCAFEDEAD);
> -
> -memset(&ib, 0, sizeof(ib));
> -r = amdgpu_ib_get(adev, NULL, 256, &ib);
> -if (r) {
> -DRM_ERROR("amdgpu: failed to get ib (%ld).\n", r);
> -goto err1;
> -}
> -
> -ib.ptr[0] = PACKET3(PACKET3_SET_UCONFIG_REG, 1);
> -ib.ptr[1] = ((scratch - PACKET3_SET_UCONFIG_REG_START));
> -ib.ptr[2] = 0xDEADBEEF;
> -ib.length_dw = 3;
> -
> -r = amdgpu_ib_schedule(ring, 1, &ib, NULL, &f);
> -if (r)
> -goto err2;
> -
> -r = dma_fence_wait_timeout(f, false, timeout);
> -if (r == 0) {
> -DRM_ERROR("amdgpu: IB test timed out.\n");
> -r = -ETIMEDOUT;
> -goto err2;
> -} else if (r < 0) {
> -DRM_ERROR("amdgpu: fence wait failed (%ld).\n", r);
> -goto err2;
> -}
> -
> -tmp = RREG32(scratch);
> -if (tmp == 0xDEADBEEF) {
> -DRM_INFO("ib test on ring %d succeeded\n", ring->idx);
> -r = 0;
> -} else {
> -DRM_ERROR("amdgpu: ib test failed 
> (scratch(0x%04X)=0x%08X)\n",
> -  scratch, tmp);
> -r = -EINVAL;
> -}
> +   struct amdgpu_device *adev = ring->adev;
> +   struct amdgpu_ib ib;
> +   struct dma_fence *f = NULL;
> +   uint32_t scratch;
> +   uint32_t tmp = 0;
> +   long r;
> +
> +   r = amdgpu_gfx_scratch_get(adev, &scratch);
> +   if (r) {
> +   DRM_ERROR("amdgpu: failed to get scratch reg (%ld).\n", r);
> +   return r;
> +   }
> +
> +   WREG32(scratch, 0xCAFEDEAD);
> +
> +   memset(&ib, 0, sizeof(ib));
> +   r = amdgpu_ib_get(adev, NULL, 256, &ib);
> +   if (r) {
> +   DRM_ERROR("amdgpu: failed to get ib (%ld).\n", r);
> +   goto err1;
> +   }
> +
> +   ib.ptr[0] = PACKET3(PACKET3_SET_UCONFIG_REG, 1);
> +   ib.ptr[1] = ((scratch - PACKET3_SET_UCONFIG_REG_START));
> +   ib.ptr[2] = 0xDEADBEEF;
> +   ib.length_dw = 3;
> +
> +   r = amdgpu_ib_schedule(ring, 1, &ib, NULL, &f);
> +   if (r)
> +   goto err2;
> +
> +   r = dma_fence_wait_timeout(f, false, timeout);
> +   if (r == 0) {
> +   DRM_ERROR("amdgpu: IB test timed out.\n");
> +   r = -ETIMEDOUT;
> +   goto err2;
> +   } else if (r < 0) {
> +   DRM_ERROR("amdgpu: fence wait failed (%ld).\n", r);
> +   goto err2;
> +   }
> +
> +   tmp = RREG32(scratch);
> +   if (tmp == 0xDEADBEEF) {
> +   DRM_INFO("ib test on ring %d succeeded\n", ring->idx);
> +   r = 0;
> +   } else {
> +   DRM_ERROR("amdgpu: ib test failed (scratch(0x%04X)=0x%08X)\n",
> + scratch, tmp);
> +   r = -EINVAL;
> +   }
>  err2:
> -amdgpu_ib_free(adev, &ib, NULL);
> -dma_fence_put(f);
> +   amdgpu_ib_free(adev, &ib, NULL);
> +   dma_fence_put(f);
>  err1:
> -amdgpu_gfx_scratch_free(adev, scr

Re: [PATCH] drm/amdgpu: drop unused df init callback

[Kuehling, Felix]

On 2019-06-21 5:33 p.m., Alex Deucher wrote:
> It was replaced with the sw_init callback so is no longer
> needed.
>
> Signed-off-by: Alex Deucher 
Reviewed-by: Felix Kuehling 
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 7118ca21aa4e..896a4bc60040 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -687,7 +687,6 @@ struct amdgpu_nbio_funcs {
>   };
>   
>   struct amdgpu_df_funcs {
> - void (*init)(struct amdgpu_device *adev);
>   void (*sw_init)(struct amdgpu_device *adev);
>   void (*enable_broadcast_mode)(struct amdgpu_device *adev,
> bool enable);
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: drop unused df init callback

[Alex Deucher]

It was replaced with the sw_init callback so is no longer
needed.

Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 7118ca21aa4e..896a4bc60040 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -687,7 +687,6 @@ struct amdgpu_nbio_funcs {
 };
 
 struct amdgpu_df_funcs {
-   void (*init)(struct amdgpu_device *adev);
void (*sw_init)(struct amdgpu_device *adev);
void (*enable_broadcast_mode)(struct amdgpu_device *adev,
  bool enable);
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: add sw_init to df_v1_7

[Kim, Jonathan]

Thanks, sorry about this.  I'll be more thorough next time.  Patch verified 
with vega20 and vega10 systems.

Jon

-Original Message-
From: Kuehling, Felix  
Sent: Friday, June 21, 2019 12:10 PM
To: amd-gfx@lists.freedesktop.org; Kim, Jonathan 
Subject: Re: [PATCH] drm/amdgpu: add sw_init to df_v1_7

On 2019-06-21 11:31 a.m., Kim, Jonathan wrote:
> change df_init to df_sw_init df 1.7 to prevent regression issues on 
> pre-vega20 products when callback is called in sw_common_sw_init.
>
> Change-Id: I4941003ea4a99ba0ea736c7ecc8800148423c379
> Signed-off-by: Jonathan Kim 

Reviewed-by: Felix Kuehling 

So your previous patch broke the build. Please at least build test your code 
before you push. Breaking the build is not acceptable. It slows down everybody 
else. I'll take a look if anything else can be cleaned up with these callbacks 
later.


> ---
>   drivers/gpu/drm/amd/amdgpu/df_v1_7.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/df_v1_7.c 
> b/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
> index 9935371db7ce..844c03868248 100644
> --- a/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
> +++ b/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
> @@ -29,7 +29,7 @@
>   
>   static u32 df_v1_7_channel_number[] = {1, 2, 0, 4, 0, 8, 0, 16, 2};
>   
> -static void df_v1_7_init (struct amdgpu_device *adev)
> +static void df_v1_7_sw_init(struct amdgpu_device *adev)
>   {
>   }
>   
> @@ -110,7 +110,7 @@ static void df_v1_7_enable_ecc_force_par_wr_rmw(struct 
> amdgpu_device *adev,
>   }
>   
>   const struct amdgpu_df_funcs df_v1_7_funcs = {
> - .init = df_v1_7_init,
> + .sw_init = df_v1_7_sw_init,
>   .enable_broadcast_mode = df_v1_7_enable_broadcast_mode,
>   .get_fb_channel_number = df_v1_7_get_fb_channel_number,
>   .get_hbm_channel_number = df_v1_7_get_hbm_channel_number,
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/1] drm/amdkfd: Add queue status indicator to HQD dump

[Kuehling, Felix]

Make it easy to distinguish unmapped, idle and busy queues in the
HQD dump.

Also remove CU mask from the HQD dump because these registers are
not per-queue, but per pipe.

Signed-off-by: Felix Kuehling 
---
 .../drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c| 35 ++--
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c | 39 +-
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c | 39 +-
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c | 35 ++--
 .../drm/amd/amdkfd/kfd_device_queue_manager.c | 40 +++
 .../gpu/drm/amd/include/kgd_kfd_interface.h   | 15 ++-
 6 files changed, 157 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
index 39ffb078beb4..4882b4112a7a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10.c
@@ -68,12 +68,14 @@ static int kgd_hqd_load(struct kgd_dev *kgd, void *mqd, 
uint32_t pipe_id,
struct mm_struct *mm);
 static int kgd_hqd_dump(struct kgd_dev *kgd,
uint32_t pipe_id, uint32_t queue_id,
-   uint32_t (**dump)[2], uint32_t *n_regs);
+   uint32_t (**dump)[2], uint32_t *n_regs,
+   enum kfd_queue_status *status);
 static int kgd_hqd_sdma_load(struct kgd_dev *kgd, void *mqd,
 uint32_t __user *wptr, struct mm_struct *mm);
 static int kgd_hqd_sdma_dump(struct kgd_dev *kgd,
 uint32_t engine_id, uint32_t queue_id,
-uint32_t (**dump)[2], uint32_t *n_regs);
+uint32_t (**dump)[2], uint32_t *n_regs,
+enum kfd_queue_status *status);
 static bool kgd_hqd_is_occupied(struct kgd_dev *kgd, uint64_t queue_address,
uint32_t pipe_id, uint32_t queue_id);
 static bool kgd_hqd_sdma_is_occupied(struct kgd_dev *kgd, void *mqd);
@@ -454,9 +456,17 @@ static int kgd_hqd_load(struct kgd_dev *kgd, void *mqd, 
uint32_t pipe_id,
 
 static int kgd_hqd_dump(struct kgd_dev *kgd,
uint32_t pipe_id, uint32_t queue_id,
-   uint32_t (**dump)[2], uint32_t *n_regs)
+   uint32_t (**dump)[2], uint32_t *n_regs,
+   enum kfd_queue_status *status)
 {
struct amdgpu_device *adev = get_amdgpu_device(kgd);
+   const uint32_t
+   hqd_active = (SOC15_REG_OFFSET(GC, 0, mmCP_HQD_ACTIVE) -
+ SOC15_REG_OFFSET(GC, 0, mmCP_MQD_BASE_ADDR)),
+   hqd_pq_rptr = (SOC15_REG_OFFSET(GC, 0, mmCP_HQD_PQ_RPTR) -
+  SOC15_REG_OFFSET(GC, 0, mmCP_MQD_BASE_ADDR)),
+   hqd_pq_wptr = (SOC15_REG_OFFSET(GC, 0, mmCP_HQD_PQ_WPTR_LO) -
+  SOC15_REG_OFFSET(GC, 0, mmCP_MQD_BASE_ADDR));
uint32_t i = 0, reg;
 #define HQD_N_REGS 56
 #define DUMP_REG(addr) do {\
@@ -481,6 +491,12 @@ static int kgd_hqd_dump(struct kgd_dev *kgd,
WARN_ON_ONCE(i != HQD_N_REGS);
*n_regs = i;
 
+   if (!(*dump)[hqd_active][1])
+   *status = KFD_QUEUE_UNMAPPED;
+   else
+   *status = (*dump)[hqd_pq_rptr][1] == (*dump)[hqd_pq_wptr][1] ?
+   KFD_QUEUE_IDLE : KFD_QUEUE_BUSY;
+
return 0;
 }
 
@@ -561,9 +577,14 @@ static int kgd_hqd_sdma_load(struct kgd_dev *kgd, void 
*mqd,
 
 static int kgd_hqd_sdma_dump(struct kgd_dev *kgd,
 uint32_t engine_id, uint32_t queue_id,
-uint32_t (**dump)[2], uint32_t *n_regs)
+uint32_t (**dump)[2], uint32_t *n_regs,
+enum kfd_queue_status *status)
 {
struct amdgpu_device *adev = get_amdgpu_device(kgd);
+   const uint32_t
+   rlc_doorbell = mmSDMA0_RLC0_DOORBELL - mmSDMA0_RLC0_RB_CNTL,
+   rlc_rb_rptr = mmSDMA0_RLC0_RB_RPTR - mmSDMA0_RLC0_RB_CNTL,
+   rlc_rb_wptr = mmSDMA0_RLC0_RB_WPTR - mmSDMA0_RLC0_RB_CNTL;
uint32_t sdma_base_addr = get_sdma_base_addr(adev, engine_id, queue_id);
uint32_t i = 0, reg;
 #undef HQD_N_REGS
@@ -590,6 +611,12 @@ static int kgd_hqd_sdma_dump(struct kgd_dev *kgd,
WARN_ON_ONCE(i != HQD_N_REGS);
*n_regs = i;
 
+   if (!(*dump)[rlc_doorbell][1])
+   *status = KFD_QUEUE_UNMAPPED;
+   else
+   *status = (*dump)[rlc_rb_rptr][1] == (*dump)[rlc_rb_wptr][1] ?
+   KFD_QUEUE_IDLE : KFD_QUEUE_BUSY;
+
return 0;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
index c6abcf72e822..10b0ec8ca852 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
@@ -104,12 +104,14 @@ static 

Re: [Intel-gfx] [PATCH 1/6] dma-buf: add dynamic DMA-buf handling v10

[Daniel Vetter]

On Fri, Jun 21, 2019 at 02:06:54PM +0200, Christian König wrote:
> Am 21.06.19 um 12:32 schrieb Daniel Vetter:
> > On Fri, Jun 21, 2019 at 11:55 AM Christian König
> >  wrote:
> > > Am 21.06.19 um 11:20 schrieb Daniel Vetter:
> > > > On Tue, Jun 18, 2019 at 01:54:50PM +0200, Christian König wrote:
> > > > [SNIP]
> > > > Imo the below semantics would be much cleaner:
> > > > 
> > > > - invalidate may add new fences
> > > > - invalidate _must_ unmap its mappings
> > > > - an unmap must wait for current fences before the mapping can be
> > > > released.
> > > > 
> > > > Imo there's no reason why unmap is special, and the only thing where we
> > > > don't use fences to gate access to resources/memory when it's in the
> > > > process of getting moved around.
> > > Well in general I want to avoid waiting for fences as much as possible.
> > > But the key point here is that this actually won't help with the problem
> > > I'm trying to solve.
> > The point of using fences is not to wait on them. I mean if you have
> > the shadow ttm bo on the lru you also don't wait for that fence to
> > retire before you insert the shadow bo onto the lru. You don't even
> > wait when you try to use that memory again, you just pipeline more
> > stuff on top.
> 
> Correct.
> 
> Ok, if I understand it correctly your suggestion is to move the
> responsibility to delay destruction of mappings until they are no longer
> used from the importer to the exporter based on the fences of the
> reservation object.
> 
> I seriously don't think that this is a good idea because you need to move
> the tracking who is using which mapping from the importer to the exporter as
> well. So duplicating quite a bunch of housekeeping.
> 
> On the other hand that we have this house keeping in the importer because we
> get it for free from TTM. But I can't think of a way other memory management
> backends would do this without keeping the sg table around either.
> 
> > In the end it will be the exact same amount of fences and waiting in
> > both solutions. One just leaks less implementationt details (at least
> > in my opinion) across the dma-buf border.
> 
> I agree that leaking implementation details over the DMA-buf border is a bad
> idea.
> 
> But I can assure you that this has absolutely nothing todo with the ghost
> object handling of TTM. ghost objects doesn't even receive an invalidation,
> they are just a possible implementation of the delayed destruction of sg
> tables.
> 
> > > > btw this is like the 2nd or 3rd time I'm typing this, haven't seen your
> > > > thoughts on this yet.
> > > Yeah, and I'm responding for the 3rd time now that you are
> > > misunderstanding why we need this here :)
> > > 
> > > Maybe I can make that clear with an example:
> > > 
> > > 1. You got a sharing between device A (exporter) and B (importer) which
> > > uses P2P.
> > > 
> > > 2. Now device C (importer) comes along and wants to use the DMA-buf
> > > object as well.
> > > 
> > > 3. The handling now figures out that we can't do P2P between device A
> > > and device C (for whatever reason).
> > > 
> > > 4. The map_attachment implementation in device driver A doesn't want to
> > > fail with -EBUSY and migrates the DMA-buf somewhere where both device A
> > > and device C can access it.
> > > 
> > > 5. This migration will result in sending an invalidation event around.
> > > And here it doesn't make sense to send this invalidation event to device
> > > C, because we know that device C is actually causing this event and
> > > doesn't have a valid mapping.
> > Hm I thought the last time around there was a different scenario, with
> > just one importer:
> > 
> > - importer has a mapping, gets an ->invalidate call.
> > - importer arranges for the mappings/usage to get torn down, maybe
> > updating fences, all from ->invalidate. But the mapping itself wont
> > disappear.
> > - exporter moves buffer to new places (for whatever reasons it felt
> > that was the thing to do).
> > - importer does another execbuf, the exporter needs to move the buffer
> > back. Again it calls ->invalidate, but on a mapping it already has
> > called ->invalidate on, and to prevent that silliness we take the
> > importer temporary off the list.
> 
> Mhm, strange I don't remember giving this explanation. It also doesn't make
> to much sense, but see below.

Yeah maybe I mixed things up somewhere. I guess you started with the first
scenario, I replied with "why don't we track this in the exporter or
dma-buf.c then?" and then the thread died out or something.
> 
> > Your scenario here is new, and iirc my suggestion back then was to
> > count the number of pending mappings so you don't go around calling
> > ->invalidate on mappings that don't exist.
> 
> Well the key point is we don't call invalidate on mappings, but we call
> invalidate on attachments.
> 
> When the invalidate on an attachment is received all the importer should at
> least start to tear down all mappings.

Hm, so either we invalidate

Re: [PATCH] drm/amdgpu: add sw_init to df_v1_7

[Kuehling, Felix]

On 2019-06-21 11:31 a.m., Kim, Jonathan wrote:
> change df_init to df_sw_init df 1.7 to prevent regression issues on pre-vega20
> products when callback is called in sw_common_sw_init.
>
> Change-Id: I4941003ea4a99ba0ea736c7ecc8800148423c379
> Signed-off-by: Jonathan Kim 

Reviewed-by: Felix Kuehling 

So your previous patch broke the build. Please at least build test your 
code before you push. Breaking the build is not acceptable. It slows 
down everybody else. I'll take a look if anything else can be cleaned up 
with these callbacks later.


> ---
>   drivers/gpu/drm/amd/amdgpu/df_v1_7.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/df_v1_7.c 
> b/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
> index 9935371db7ce..844c03868248 100644
> --- a/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
> +++ b/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
> @@ -29,7 +29,7 @@
>   
>   static u32 df_v1_7_channel_number[] = {1, 2, 0, 4, 0, 8, 0, 16, 2};
>   
> -static void df_v1_7_init (struct amdgpu_device *adev)
> +static void df_v1_7_sw_init(struct amdgpu_device *adev)
>   {
>   }
>   
> @@ -110,7 +110,7 @@ static void df_v1_7_enable_ecc_force_par_wr_rmw(struct 
> amdgpu_device *adev,
>   }
>   
>   const struct amdgpu_df_funcs df_v1_7_funcs = {
> - .init = df_v1_7_init,
> + .sw_init = df_v1_7_sw_init,
>   .enable_broadcast_mode = df_v1_7_enable_broadcast_mode,
>   .get_fb_channel_number = df_v1_7_get_fb_channel_number,
>   .get_hbm_channel_number = df_v1_7_get_hbm_channel_number,
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Michel Dänzer]

On 2019-06-21 5:44 p.m., Daniel Vetter wrote:
> On Fri, Jun 21, 2019 at 05:15:22PM +0200, Michel Dänzer wrote:
>> On 2019-06-21 1:50 p.m., Daniel Vetter wrote:
>>> On Fri, Jun 21, 2019 at 1:37 PM Christian König
>>>  wrote:
 Am 21.06.19 um 13:03 schrieb Daniel Vetter:
> So if you want to depracate render functionality on primary nodes, you
> _have_ to do that hiding in userspace. Or you'll break a lot of
> compositors everywhere. Just testing -amdgpu doesn't really prove
> anything here. So you won't move the larger ecosystem with this at
> all, that ship sailed.

 So what else can we do? That sounds like you suggest we should
 completely drop render node functionality.

 I mean it's not my preferred option, but certainly something that
 everybody could do.

 My primary concern is that we somehow need to get rid of thinks like GEM
 flink and all that other crufty stuff we still have around on the
 primary node (we should probably make a list of that).

 Switching everything over to render nodes just sounded like the best
 alternative so far to archive that.
>>>
>>> tbh I do like that plan too, but it's a lot more work. And I think to
>>> have any push whatsoever we probably need to roll it out in gbm as a
>>> hack to keep things going. But probably not enough.
>>>
>>> I also think that at least some compositors will bother to do the
>>> right thing, and actually bother with render nodes and all that
>>> correctly. It's just that there's way more which dont.
>>
>> With amdgpu, we can make userspace always use render nodes for rendering
>> with changes to libdrm_amdgpu/radeonsi/xf86-video-amdgpu (and maybe some
>> amdgpu-pro components) only. No GBM/compositor changes needed.
> 
> This is a very non-exhaustive list of userspace that runs on your driver
> ... This wayland compositor thing, actually shipping now, and there's many :-)

You don't seem to understand what I wrote. Everything will work at least
as well as now, without any other changes.


> Once we picked a color it's a simple technical matter of how to roll
> it out, using Kconfig options, or only enabling on new hw, or "merge
> and fix the regressions as they come" or any of the other well proven
> paths forward.

 Yeah, the problem is I don't see an option which currently works for
 everyone.

 I absolutely need a grace time of multiple years until we can apply this
 to amdgpu/radeon.
>>>
>>> Yeah that's what I meant with "pick a color, pick a way to roll it
>>> out". "enable for new hw, roll out years and years later" is one of
>>> the options for roll out.
>>
>> One gotcha being that generic userspace such as the Xorg modesetting
>> driver may still try to use phased out functionality such as DRI2 with
>> new hardware.
> 
> There's a lot more generic userspace than -modesetting.

That is affected by phasing out DRI2 related functionality? (I thought
that was the context of this sub-sub-thread)


> That was the entire point of kms, and it succeed really well. That's
> why I don't think amdgpu doing their own flavour pick is going to help
> anyone here,
Other drivers are welcome to emulate amdgpu's design making the above
possible. :)


> except maybe if all you care about is the amd stand-alone> stack only.
> But then why do you bother with this upstream stuff at all> ...

I'm afraid you've lost me here.


-- 
Earthling Michel Dänzer   |  https://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Daniel Vetter]

On Fri, Jun 21, 2019 at 05:15:22PM +0200, Michel Dänzer wrote:
> On 2019-06-21 1:50 p.m., Daniel Vetter wrote:
> > On Fri, Jun 21, 2019 at 1:37 PM Christian König
> >  wrote:
> >> Am 21.06.19 um 13:03 schrieb Daniel Vetter:
> >>> So if you want to depracate render functionality on primary nodes, you
> >>> _have_ to do that hiding in userspace. Or you'll break a lot of
> >>> compositors everywhere. Just testing -amdgpu doesn't really prove
> >>> anything here. So you won't move the larger ecosystem with this at
> >>> all, that ship sailed.
> >>
> >> So what else can we do? That sounds like you suggest we should
> >> completely drop render node functionality.
> >>
> >> I mean it's not my preferred option, but certainly something that
> >> everybody could do.
> >>
> >> My primary concern is that we somehow need to get rid of thinks like GEM
> >> flink and all that other crufty stuff we still have around on the
> >> primary node (we should probably make a list of that).
> >>
> >> Switching everything over to render nodes just sounded like the best
> >> alternative so far to archive that.
> > 
> > tbh I do like that plan too, but it's a lot more work. And I think to
> > have any push whatsoever we probably need to roll it out in gbm as a
> > hack to keep things going. But probably not enough.
> > 
> > I also think that at least some compositors will bother to do the
> > right thing, and actually bother with render nodes and all that
> > correctly. It's just that there's way more which dont.
> 
> With amdgpu, we can make userspace always use render nodes for rendering
> with changes to libdrm_amdgpu/radeonsi/xf86-video-amdgpu (and maybe some
> amdgpu-pro components) only. No GBM/compositor changes needed.

This is a very non-exhaustive list of userspace that runs on your driver
... This wayland compositor thing, actually shipping now, and there's many :-)

> >>> At that point this all feels like a bikeshed, and for a bikeshed I
> >>> don't care what the color is we pick, as long as they're all painted
> >>> the same.
> 
> Then some sheds shouldn't have been re-painted without DRM_AUTH already...

Christian proposed that and then didn't feel like reverting it, plus vc4,
and tegra never bothered with it. There's quite a bit more than the tail
out of this particular bag already.

> >>> Once we picked a color it's a simple technical matter of how to roll
> >>> it out, using Kconfig options, or only enabling on new hw, or "merge
> >>> and fix the regressions as they come" or any of the other well proven
> >>> paths forward.
> >>
> >> Yeah, the problem is I don't see an option which currently works for
> >> everyone.
> >>
> >> I absolutely need a grace time of multiple years until we can apply this
> >> to amdgpu/radeon.
> > 
> > Yeah that's what I meant with "pick a color, pick a way to roll it
> > out". "enable for new hw, roll out years and years later" is one of
> > the options for roll out.
> 
> One gotcha being that generic userspace such as the Xorg modesetting
> driver may still try to use phased out functionality such as DRI2 with
> new hardware.

There's a lot more generic userspace than -modesetting. That was the
entire point of kms, and it succeed really well. That's why I don't think
amdgpu doing their own flavour pick is going to help anyone here, except
maybe if all you care about is the amd stand-alone stack only. But then
why do you bother with this upstream stuff at all ...

> >> How about this:
> >> 1. We keep DRM_AUTH around for amdgpu/radeon for now.
> >> 2. We add a Kconfig option to ignore DRM_AUTH, currently default to N.
> >> This also works as a workaround for old RADV.
> >> 3. We start to work on further removing old cruft from the primary node.
> >> Possible the Kconfig option I suggested to disable GEM flink.
> > 
> > Hm I'm not worried about flink really. It's gem_open which is the
> > security gap, and that one has to stay master-only forever.
> 
> GEM_OPEN is used by DRI2 clients, it can't be master-only. It's probably
> one of the main reasons for the existence of DRM_AUTH.

Oh sweet I forgot we're allowing this both ways :-/ Well doesn't change that
much really, once we break one of these the other isn't useful anymore
either.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Daniel Vetter]

On Fri, Jun 21, 2019 at 01:00:17PM +, Koenig, Christian wrote:
> Am 21.06.19 um 14:47 schrieb Emil Velikov:
> > On 2019/06/21, Koenig, Christian wrote:
> >> Am 21.06.19 um 13:58 schrieb Emil Velikov:
> >>> On 2019/06/21, Koenig, Christian wrote:
>  Am 21.06.19 um 12:53 schrieb Emil Velikov:
> > On 2019/06/21, Koenig, Christian wrote:
> >> [SNIP]
> >> Well partially. That RADV broke is unfortunate, but we have done so 
> >> many
> >> customized userspace stuff both based on Mesa/DDX as well as closed
> >> source components that it is just highly likely that we would break
> >> something else as well.
> >>
> > As an engineer I like to work with tangibles. The highly likely part is
> > probable, but as mentioned multiple times the series allows for a _dead_
> > trivial way to address any such problems.
>  Well to clarify my concern is that this won't be so trivial.
> 
>  You implemented on workaround for one specific case and it is perfectly
>  possible that for other cases we would have to completely revert the
>  removal of DRM_AUTH.
> 
> >>> I would encourage you to take a closer look at my patch and point out
> >>> how parcicular cases cannot be handled.
> >> Well the last time I looked it only blocked the first call to the IOCTL.
> >>
> > Hmm interesting, you're replied to my patch without even reading it :'-(
> 
> Well I've NAKed the series because of the exposure of the render node 
> functionality
> 
> > Can you please give it a close look and reply inline?
> >
> > [1] https://lists.freedesktop.org/archives/dri-devel/2019-May/219238.html
> 
> So the workaround no longer just blocks the first IOCTL.
> 
> But then the question is why don't you just keep the DRM_AUTH flag 
> instead of adding the same functionality with a new one?
> 
> >>>   From your experiense, do user-space developers care about doing (or
> >>> generally do) the right thing?
> >> No, not at all. Except for Marek and Michel they just take what works
> >> and even Marek is often short-cutting on this.
> >>
> > Interesting, guess I should have asked this question from the start.
> 
> Well sounds like you don't have to work with a closed source driver 
> team. But as I said I honestly would do the same as user space developer.

From an upstream kernel pov I've never cared about the blobs. I don't
upstream should terribly start caring about blobs - if they ship hacks
that don't reflect what the open stack does, their problem.
 
Speaking as someone who's pissed off closed source driver teams to no end.
I'm happy to be of service here too :-)

> I mean it's really a bunch of more code to maintain, and getting rid of 
> code is always less work in the long term then keeping it.
> 
> That kernel developers scream: No no, please don't do that we want to 
> keep using it is completely irrelevant for this.

Jokes aside, I think we should look at what's the right thing to do
looking at open source only, and if that gets the blobby folks shafted,
well so be it. Really not my problem, and I'm pretty sure Dave doesn't
care one hair of an inch more.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: add sw_init to df_v1_7

[Kim, Jonathan]

change df_init to df_sw_init df 1.7 to prevent regression issues on pre-vega20
products when callback is called in sw_common_sw_init.

Change-Id: I4941003ea4a99ba0ea736c7ecc8800148423c379
Signed-off-by: Jonathan Kim 
---
 drivers/gpu/drm/amd/amdgpu/df_v1_7.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/df_v1_7.c 
b/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
index 9935371db7ce..844c03868248 100644
--- a/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
+++ b/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
@@ -29,7 +29,7 @@
 
 static u32 df_v1_7_channel_number[] = {1, 2, 0, 4, 0, 8, 0, 16, 2};
 
-static void df_v1_7_init (struct amdgpu_device *adev)
+static void df_v1_7_sw_init(struct amdgpu_device *adev)
 {
 }
 
@@ -110,7 +110,7 @@ static void df_v1_7_enable_ecc_force_par_wr_rmw(struct 
amdgpu_device *adev,
 }
 
 const struct amdgpu_df_funcs df_v1_7_funcs = {
-   .init = df_v1_7_init,
+   .sw_init = df_v1_7_sw_init,
.enable_broadcast_mode = df_v1_7_enable_broadcast_mode,
.get_fb_channel_number = df_v1_7_get_fb_channel_number,
.get_hbm_channel_number = df_v1_7_get_hbm_channel_number,
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Michel Dänzer]

On 2019-06-21 1:58 p.m., Emil Velikov wrote:
> On 2019/06/21, Koenig, Christian wrote:
>> Am 21.06.19 um 12:53 schrieb Emil Velikov:
>>> On 2019/06/21, Koenig, Christian wrote:
> 
> In particular, that user-space will "remove" render nodes.
 Yes, that is my main concern here. You basically make render nodes
 superfluously. That gives userspace all legitimization to also remove
 support for them. That is not stupid or evil or whatever, userspace
 would just follow the kernel design.

>>> Do you have an example of userspace doing such an extremely silly thing?
>>> It does seem like suspect you're a couple of steps beyond overcautious,
>>> perhaps rightfully so. Maybe you've seen some closed-source user-space
>>> going crazy? Or any other projects?
>>
>> The key point is that I don't think this is silly or strange or crazy at 
>> all. See the kernel defines the interface userspace can and should use.
>>
>> When the kernel defines that everything will work with the primary node 
>> it is perfectly valid for userspace to drop support for the render node.
>>
>> I mean why should they keep this? Just because we tell them not to do this?
>>
> From your experiense, do user-space developers care about doing (or
> generally do) the right thing?
> 
> In either case, as pointed already the cat is out of the bag - has been
> for years, and if developers did behave as you describe them they would
> have "removed" render nodes already.

That may be the case with userspace specific to DRM_AUTH-less kernel
drivers, but such userspace couldn't work with all the other drivers. So
I'd argue that while the bag may be open and the cat's tail may even be
sticking out already, the cat is still firmly in the bag. :)


-- 
Earthling Michel Dänzer   |  https://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Michel Dänzer]

On 2019-06-21 1:50 p.m., Daniel Vetter wrote:
> On Fri, Jun 21, 2019 at 1:37 PM Christian König
>  wrote:
>> Am 21.06.19 um 13:03 schrieb Daniel Vetter:
>>> So if you want to depracate render functionality on primary nodes, you
>>> _have_ to do that hiding in userspace. Or you'll break a lot of
>>> compositors everywhere. Just testing -amdgpu doesn't really prove
>>> anything here. So you won't move the larger ecosystem with this at
>>> all, that ship sailed.
>>
>> So what else can we do? That sounds like you suggest we should
>> completely drop render node functionality.
>>
>> I mean it's not my preferred option, but certainly something that
>> everybody could do.
>>
>> My primary concern is that we somehow need to get rid of thinks like GEM
>> flink and all that other crufty stuff we still have around on the
>> primary node (we should probably make a list of that).
>>
>> Switching everything over to render nodes just sounded like the best
>> alternative so far to archive that.
> 
> tbh I do like that plan too, but it's a lot more work. And I think to
> have any push whatsoever we probably need to roll it out in gbm as a
> hack to keep things going. But probably not enough.
> 
> I also think that at least some compositors will bother to do the
> right thing, and actually bother with render nodes and all that
> correctly. It's just that there's way more which dont.

With amdgpu, we can make userspace always use render nodes for rendering
with changes to libdrm_amdgpu/radeonsi/xf86-video-amdgpu (and maybe some
amdgpu-pro components) only. No GBM/compositor changes needed.


>>> At that point this all feels like a bikeshed, and for a bikeshed I
>>> don't care what the color is we pick, as long as they're all painted
>>> the same.

Then some sheds shouldn't have been re-painted without DRM_AUTH already...


>>> Once we picked a color it's a simple technical matter of how to roll
>>> it out, using Kconfig options, or only enabling on new hw, or "merge
>>> and fix the regressions as they come" or any of the other well proven
>>> paths forward.
>>
>> Yeah, the problem is I don't see an option which currently works for
>> everyone.
>>
>> I absolutely need a grace time of multiple years until we can apply this
>> to amdgpu/radeon.
> 
> Yeah that's what I meant with "pick a color, pick a way to roll it
> out". "enable for new hw, roll out years and years later" is one of
> the options for roll out.

One gotcha being that generic userspace such as the Xorg modesetting
driver may still try to use phased out functionality such as DRI2 with
new hardware.


>> How about this:
>> 1. We keep DRM_AUTH around for amdgpu/radeon for now.
>> 2. We add a Kconfig option to ignore DRM_AUTH, currently default to N.
>> This also works as a workaround for old RADV.
>> 3. We start to work on further removing old cruft from the primary node.
>> Possible the Kconfig option I suggested to disable GEM flink.
> 
> Hm I'm not worried about flink really. It's gem_open which is the
> security gap, and that one has to stay master-only forever.

GEM_OPEN is used by DRI2 clients, it can't be master-only. It's probably
one of the main reasons for the existence of DRM_AUTH.


-- 
Earthling Michel Dänzer   |  https://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] Fix VM handling for VMID 0 w.r.t. SYSTEM_APERTURE

[Christian König]

Am 20.06.19 um 19:44 schrieb StDenis, Tom:

Signed-off-by: Tom St Denis 


Acked-by: Christian König 


---
  src/lib/read_vram.c | 47 +
  1 file changed, 43 insertions(+), 4 deletions(-)

diff --git a/src/lib/read_vram.c b/src/lib/read_vram.c
index 131cec3..812de96 100644
--- a/src/lib/read_vram.c
+++ b/src/lib/read_vram.c
@@ -300,7 +300,7 @@ static int umr_access_vram_ai(struct umr_asic *asic, 
uint32_t vmid,
uint64_t start_addr, page_table_start_addr, page_table_base_addr,
 page_table_size, pte_idx, pde_idx, pte_entry, pde_entry,
 pde_address, vga_base_address, vm_fb_offset, vm_fb_base,
-va_mask, offset_mask;
+va_mask, offset_mask, system_aperture_low, 
system_aperture_high;
uint32_t chunk_size, tmp;
int pde_cnt, current_depth, page_table_depth, first;
struct {
@@ -312,7 +312,10 @@ static int umr_access_vram_ai(struct umr_asic *asic, 
uint32_t vmid,
mmVM_CONTEXTx_PAGE_TABLE_BASE_ADDR_HI32,
mmVGA_MEMORY_BASE_ADDRESS,
mmVGA_MEMORY_BASE_ADDRESS_HIGH,
-   mmMC_VM_FB_OFFSET;
+   mmMC_VM_FB_OFFSET,
+   mmMC_VM_MX_L1_TLB_CNTL,
+   mmMC_VM_SYSTEM_APERTURE_LOW_ADDR,
+   mmMC_VM_SYSTEM_APERTURE_HIGH_ADDR;
} registers;
struct {
uint64_t
@@ -381,6 +384,11 @@ static int umr_access_vram_ai(struct umr_asic *asic, 
uint32_t vmid,
}
  
  	// read vm registers

+   registers.mmMC_VM_SYSTEM_APERTURE_HIGH_ADDR = umr_read_reg_by_name_by_ip(asic, 
hub, "mmMC_VM_SYSTEM_APERTURE_HIGH_ADDR");
+   registers.mmMC_VM_SYSTEM_APERTURE_LOW_ADDR = umr_read_reg_by_name_by_ip(asic, 
hub, "mmMC_VM_SYSTEM_APERTURE_LOW_ADDR");
+   system_aperture_low = ((uint64_t)registers.mmMC_VM_SYSTEM_APERTURE_LOW_ADDR) 
<< 18;
+   system_aperture_high = 
((uint64_t)registers.mmMC_VM_SYSTEM_APERTURE_HIGH_ADDR) << 18;
+   registers.mmMC_VM_MX_L1_TLB_CNTL = umr_read_reg_by_name_by_ip(asic, hub, 
"mmMC_VM_MX_L1_TLB_CNTL");
sprintf(buf, "mmVM_CONTEXT%" PRIu32 "_PAGE_TABLE_START_ADDR_LO32", 
vmid);
registers.mmVM_CONTEXTx_PAGE_TABLE_START_ADDR_LO32 = 
umr_read_reg_by_name_by_ip(asic, hub, buf);
page_table_start_addr = 
(uint64_t)registers.mmVM_CONTEXTx_PAGE_TABLE_START_ADDR_LO32 << 12;
@@ -425,7 +433,10 @@ static int umr_access_vram_ai(struct umr_asic *asic, 
uint32_t vmid,
"mmVGA_MEMORY_BASE_ADDRESS=0x%" PRIx32 "\n"
"mmVGA_MEMORY_BASE_ADDRESS_HIGH=0x%" PRIx32 "\n"
"mmMC_VM_FB_OFFSET=0x%" PRIx32 "\n"
-   "mmMC_VM_FB_LOCATION_BASE=0x%" PRIx64 "\n",
+   "mmMC_VM_FB_LOCATION_BASE=0x%" PRIx64 "\n"
+   "mmMC_VM_MX_L1_TLB_CNTL=0x%" PRIx32 "\n"
+   "mmMC_VM_SYSTEM_APERTURE_LOW_ADDR=0x%" PRIx32 
"\n"
+   "mmMC_VM_SYSTEM_APERTURE_HIGH_ADDR=0x%" PRIx32 
"\n",
vmid, 
registers.mmVM_CONTEXTx_PAGE_TABLE_START_ADDR_LO32,
vmid, 
registers.mmVM_CONTEXTx_PAGE_TABLE_START_ADDR_HI32,
vmid, registers.mmVM_CONTEXTx_PAGE_TABLE_BASE_ADDR_LO32,
@@ -434,10 +445,38 @@ static int umr_access_vram_ai(struct umr_asic *asic, 
uint32_t vmid,
registers.mmVGA_MEMORY_BASE_ADDRESS,
registers.mmVGA_MEMORY_BASE_ADDRESS_HIGH,
registers.mmMC_VM_FB_OFFSET,
-   vm_fb_base);
+   vm_fb_base,
+   registers.mmMC_VM_MX_L1_TLB_CNTL,
+   registers.mmMC_VM_SYSTEM_APERTURE_LOW_ADDR,
+   registers.mmMC_VM_SYSTEM_APERTURE_HIGH_ADDR
+   );
  
  	// transform page_table_base

page_table_base_addr -= vm_fb_offset;
+
+   if (vmid == 0) {
+   uint32_t sam = umr_bitslice_reg_by_name_by_ip(asic, hub, 
"mmMC_VM_MX_L1_TLB_CNTL", "SYSTEM_ACCESS_MODE", 
registers.mmMC_VM_MX_L1_TLB_CNTL);
+   // addresses in VMID0 need special handling w.r.t. 
PAGE_TABLE_START_ADDR
+   switch (sam) {
+   case 0: // physical access
+   return umr_access_vram(asic, UMR_LINEAR_HUB, 
address, size, dst, write_en);
+   case 1: // always VM access
+   break;
+   case 2: // inside system aperture is mapped, otherwise 
unmapped
+   if (!(address >= system_aperture_low && address 
< system_aperture_high))
+   return umr_access_vram(asic, 
UMR_LINEAR_HUB, address, size, dst, write_en);
+   break;
+

Re: [PATCH] drm/amd/powerplay: no memory activity support on Vega10

[Deucher, Alexander]

Maybe it's dependent on the SMU firwmare version?

Alex

From: Russell, Kent
Sent: Friday, June 21, 2019 9:54 AM
To: Deucher, Alexander; Quan, Evan; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH] drm/amd/powerplay: no memory activity support on Vega10


It works on my Fiji card. Maybe Vega10 functionality is just broken in this 
regard?



Kent



From: amd-gfx  On Behalf Of Deucher, 
Alexander
Sent: Friday, June 21, 2019 9:42 AM
To: Quan, Evan ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/powerplay: no memory activity support on Vega10



Is this supported on smu7 parts as well?  Might be better to just enable it on 
the specific asics that support it.  I think it might just be vega20.



Alex



From: amd-gfx 
mailto:amd-gfx-boun...@lists.freedesktop.org>>
 on behalf of Evan Quan mailto:evan.q...@amd.com>>
Sent: Thursday, June 20, 2019 10:07 PM
To: amd-gfx@lists.freedesktop.org
Cc: Quan, Evan
Subject: [PATCH] drm/amd/powerplay: no memory activity support on Vega10



Make mem_busy_percent sysfs interface invisible on Vega10.

Change-Id: Ie39c3217b497a110b0b16e1b08033029bdcf2fc8
Signed-off-by: Evan Quan mailto:evan.q...@amd.com>>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 7ed84736ccc9..bcf6e089dc2e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -2945,7 +2945,8 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
 return ret;
 }
 /* APU does not have its own dedicated memory */
-   if (!(adev->flags & AMD_IS_APU)) {
+   if (!(adev->flags & AMD_IS_APU) &&
+(adev->asic_type != CHIP_VEGA10)) {
 ret = device_create_file(adev->dev,
 &dev_attr_mem_busy_percent);
 if (ret) {
@@ -3025,7 +3026,8 @@ void amdgpu_pm_sysfs_fini(struct amdgpu_device *adev)
 device_remove_file(adev->dev,
 &dev_attr_pp_od_clk_voltage);
 device_remove_file(adev->dev, &dev_attr_gpu_busy_percent);
-   if (!(adev->flags & AMD_IS_APU))
+   if (!(adev->flags & AMD_IS_APU) &&
+(adev->asic_type != CHIP_VEGA10))
 device_remove_file(adev->dev, &dev_attr_mem_busy_percent);
 if (!(adev->flags & AMD_IS_APU))
 device_remove_file(adev->dev, &dev_attr_pcie_bw);
--
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: add sw_init to df_v1_7

[Deucher, Alexander]

If both init and sw_init are empty, we don't need both.  Just rename the init 
callback to sw_init.

Alex

From: amd-gfx  on behalf of Kim, 
Jonathan 
Sent: Friday, June 21, 2019 5:45 AM
To: amd-gfx@lists.freedesktop.org
Cc: Kim, Jonathan
Subject: [PATCH] drm/amdgpu: add sw_init to df_v1_7

add df sw init to df 1.7 function to prevent regression issues on pre-vega20
products.

Change-Id: I4941003ea4a99ba0ea736c7ecc8800148423c379
Signed-off-by: Jonathan Kim 
---
 drivers/gpu/drm/amd/amdgpu/df_v1_7.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/df_v1_7.c 
b/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
index 9935371db7ce..335f2c02878f 100644
--- a/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
+++ b/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
@@ -33,6 +33,10 @@ static void df_v1_7_init (struct amdgpu_device *adev)
 {
 }

+static void df_v1_7_sw_init(struct amdgpu_device *adev)
+{
+}
+
 static void df_v1_7_enable_broadcast_mode(struct amdgpu_device *adev,
   bool enable)
 {
@@ -111,6 +115,7 @@ static void df_v1_7_enable_ecc_force_par_wr_rmw(struct 
amdgpu_device *adev,

 const struct amdgpu_df_funcs df_v1_7_funcs = {
 .init = df_v1_7_init,
+   .sw_init = df_v1_7_sw_init,
 .enable_broadcast_mode = df_v1_7_enable_broadcast_mode,
 .get_fb_channel_number = df_v1_7_get_fb_channel_number,
 .get_hbm_channel_number = df_v1_7_get_hbm_channel_number,
--
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amd/powerplay: no memory activity support on Vega10

[Russell, Kent]

It works on my Fiji card. Maybe Vega10 functionality is just broken in this 
regard?

Kent

From: amd-gfx  On Behalf Of Deucher, 
Alexander
Sent: Friday, June 21, 2019 9:42 AM
To: Quan, Evan ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/powerplay: no memory activity support on Vega10

Is this supported on smu7 parts as well?  Might be better to just enable it on 
the specific asics that support it.  I think it might just be vega20.

Alex

From: amd-gfx 
mailto:amd-gfx-boun...@lists.freedesktop.org>>
 on behalf of Evan Quan mailto:evan.q...@amd.com>>
Sent: Thursday, June 20, 2019 10:07 PM
To: amd-gfx@lists.freedesktop.org
Cc: Quan, Evan
Subject: [PATCH] drm/amd/powerplay: no memory activity support on Vega10

Make mem_busy_percent sysfs interface invisible on Vega10.

Change-Id: Ie39c3217b497a110b0b16e1b08033029bdcf2fc8
Signed-off-by: Evan Quan mailto:evan.q...@amd.com>>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 7ed84736ccc9..bcf6e089dc2e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -2945,7 +2945,8 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
 return ret;
 }
 /* APU does not have its own dedicated memory */
-   if (!(adev->flags & AMD_IS_APU)) {
+   if (!(adev->flags & AMD_IS_APU) &&
+(adev->asic_type != CHIP_VEGA10)) {
 ret = device_create_file(adev->dev,
 &dev_attr_mem_busy_percent);
 if (ret) {
@@ -3025,7 +3026,8 @@ void amdgpu_pm_sysfs_fini(struct amdgpu_device *adev)
 device_remove_file(adev->dev,
 &dev_attr_pp_od_clk_voltage);
 device_remove_file(adev->dev, &dev_attr_gpu_busy_percent);
-   if (!(adev->flags & AMD_IS_APU))
+   if (!(adev->flags & AMD_IS_APU) &&
+(adev->asic_type != CHIP_VEGA10))
 device_remove_file(adev->dev, &dev_attr_mem_busy_percent);
 if (!(adev->flags & AMD_IS_APU))
 device_remove_file(adev->dev, &dev_attr_pcie_bw);
--
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/powerplay: no memory activity support on Vega10

[Deucher, Alexander]

Is this supported on smu7 parts as well?  Might be better to just enable it on 
the specific asics that support it.  I think it might just be vega20.

Alex

From: amd-gfx  on behalf of Evan Quan 

Sent: Thursday, June 20, 2019 10:07 PM
To: amd-gfx@lists.freedesktop.org
Cc: Quan, Evan
Subject: [PATCH] drm/amd/powerplay: no memory activity support on Vega10

Make mem_busy_percent sysfs interface invisible on Vega10.

Change-Id: Ie39c3217b497a110b0b16e1b08033029bdcf2fc8
Signed-off-by: Evan Quan 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 7ed84736ccc9..bcf6e089dc2e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -2945,7 +2945,8 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
 return ret;
 }
 /* APU does not have its own dedicated memory */
-   if (!(adev->flags & AMD_IS_APU)) {
+   if (!(adev->flags & AMD_IS_APU) &&
+(adev->asic_type != CHIP_VEGA10)) {
 ret = device_create_file(adev->dev,
 &dev_attr_mem_busy_percent);
 if (ret) {
@@ -3025,7 +3026,8 @@ void amdgpu_pm_sysfs_fini(struct amdgpu_device *adev)
 device_remove_file(adev->dev,
 &dev_attr_pp_od_clk_voltage);
 device_remove_file(adev->dev, &dev_attr_gpu_busy_percent);
-   if (!(adev->flags & AMD_IS_APU))
+   if (!(adev->flags & AMD_IS_APU) &&
+(adev->asic_type != CHIP_VEGA10))
 device_remove_file(adev->dev, &dev_attr_mem_busy_percent);
 if (!(adev->flags & AMD_IS_APU))
 device_remove_file(adev->dev, &dev_attr_pcie_bw);
--
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Koenig, Christian]

Am 21.06.19 um 14:47 schrieb Emil Velikov:
> On 2019/06/21, Koenig, Christian wrote:
>> Am 21.06.19 um 13:58 schrieb Emil Velikov:
>>> On 2019/06/21, Koenig, Christian wrote:
 Am 21.06.19 um 12:53 schrieb Emil Velikov:
> On 2019/06/21, Koenig, Christian wrote:
>> [SNIP]
>> Well partially. That RADV broke is unfortunate, but we have done so many
>> customized userspace stuff both based on Mesa/DDX as well as closed
>> source components that it is just highly likely that we would break
>> something else as well.
>>
> As an engineer I like to work with tangibles. The highly likely part is
> probable, but as mentioned multiple times the series allows for a _dead_
> trivial way to address any such problems.
 Well to clarify my concern is that this won't be so trivial.

 You implemented on workaround for one specific case and it is perfectly
 possible that for other cases we would have to completely revert the
 removal of DRM_AUTH.

>>> I would encourage you to take a closer look at my patch and point out
>>> how parcicular cases cannot be handled.
>> Well the last time I looked it only blocked the first call to the IOCTL.
>>
> Hmm interesting, you're replied to my patch without even reading it :'-(

Well I've NAKed the series because of the exposure of the render node 
functionality

> Can you please give it a close look and reply inline?
>
> [1] https://lists.freedesktop.org/archives/dri-devel/2019-May/219238.html

So the workaround no longer just blocks the first IOCTL.

But then the question is why don't you just keep the DRM_AUTH flag 
instead of adding the same functionality with a new one?

>>>   From your experiense, do user-space developers care about doing (or
>>> generally do) the right thing?
>> No, not at all. Except for Marek and Michel they just take what works
>> and even Marek is often short-cutting on this.
>>
> Interesting, guess I should have asked this question from the start.

Well sounds like you don't have to work with a closed source driver 
team. But as I said I honestly would do the same as user space developer.

I mean it's really a bunch of more code to maintain, and getting rid of 
code is always less work in the long term then keeping it.

That kernel developers scream: No no, please don't do that we want to 
keep using it is completely irrelevant for this.

Christian.

>
> Thanks
> Emil

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/21, Koenig, Christian wrote:
> Am 21.06.19 um 13:58 schrieb Emil Velikov:
> > On 2019/06/21, Koenig, Christian wrote:
> >> Am 21.06.19 um 12:53 schrieb Emil Velikov:
> >>> On 2019/06/21, Koenig, Christian wrote:
>  [SNIP]
>  Well partially. That RADV broke is unfortunate, but we have done so many
>  customized userspace stuff both based on Mesa/DDX as well as closed
>  source components that it is just highly likely that we would break
>  something else as well.
> 
> >>> As an engineer I like to work with tangibles. The highly likely part is
> >>> probable, but as mentioned multiple times the series allows for a _dead_
> >>> trivial way to address any such problems.
> >> Well to clarify my concern is that this won't be so trivial.
> >>
> >> You implemented on workaround for one specific case and it is perfectly
> >> possible that for other cases we would have to completely revert the
> >> removal of DRM_AUTH.
> >>
> > I would encourage you to take a closer look at my patch and point out
> > how parcicular cases cannot be handled.
> 
> Well the last time I looked it only blocked the first call to the IOCTL.
> 
Hmm interesting, you're replied to my patch without even reading it :'-(
Can you please give it a close look and reply inline?

[1] https://lists.freedesktop.org/archives/dri-devel/2019-May/219238.html

> >  From your experiense, do user-space developers care about doing (or
> > generally do) the right thing?
> 
> No, not at all. Except for Marek and Michel they just take what works 
> and even Marek is often short-cutting on this.
> 
Interesting, guess I should have asked this question from the start.

Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Koenig, Christian]

Am 21.06.19 um 13:58 schrieb Emil Velikov:
> On 2019/06/21, Koenig, Christian wrote:
>> Am 21.06.19 um 12:53 schrieb Emil Velikov:
>>> On 2019/06/21, Koenig, Christian wrote:
 [SNIP]
 Well partially. That RADV broke is unfortunate, but we have done so many
 customized userspace stuff both based on Mesa/DDX as well as closed
 source components that it is just highly likely that we would break
 something else as well.

>>> As an engineer I like to work with tangibles. The highly likely part is
>>> probable, but as mentioned multiple times the series allows for a _dead_
>>> trivial way to address any such problems.
>> Well to clarify my concern is that this won't be so trivial.
>>
>> You implemented on workaround for one specific case and it is perfectly
>> possible that for other cases we would have to completely revert the
>> removal of DRM_AUTH.
>>
> I would encourage you to take a closer look at my patch and point out
> how parcicular cases cannot be handled.

Well the last time I looked it only blocked the first call to the IOCTL.

If that is no longer the case then what is the actual difference to 
DRM_AUTH+DRM_ALLOW_RENDER?

> In particular, that user-space will "remove" render nodes.
 Yes, that is my main concern here. You basically make render nodes
 superfluously. That gives userspace all legitimization to also remove
 support for them. That is not stupid or evil or whatever, userspace
 would just follow the kernel design.

>>> Do you have an example of userspace doing such an extremely silly thing?
>>> It does seem like suspect you're a couple of steps beyond overcautious,
>>> perhaps rightfully so. Maybe you've seen some closed-source user-space
>>> going crazy? Or any other projects?
>> The key point is that I don't think this is silly or strange or crazy at
>> all. See the kernel defines the interface userspace can and should use.
>>
>> When the kernel defines that everything will work with the primary node
>> it is perfectly valid for userspace to drop support for the render node.
>>
>> I mean why should they keep this? Just because we tell them not to do this?
>>
>  From your experiense, do user-space developers care about doing (or
> generally do) the right thing?

No, not at all. Except for Marek and Michel they just take what works 
and even Marek is often short-cutting on this.

> In either case, as pointed already the cat is out of the bag - has been
> for years, and if developers did behave as you describe them they would
> have "removed" render nodes already.

Currently render nodes are mandatory because they are needed for 
headless operation.

E.g. we have a whole bunch of customers which do transcoding with that 
on GPUs which don't even have a CRTC or even X running.

Regards,
Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-gfx] [PATCH 1/6] dma-buf: add dynamic DMA-buf handling v10

[Christian König]

Am 21.06.19 um 12:32 schrieb Daniel Vetter:

On Fri, Jun 21, 2019 at 11:55 AM Christian König
 wrote:

Am 21.06.19 um 11:20 schrieb Daniel Vetter:

On Tue, Jun 18, 2019 at 01:54:50PM +0200, Christian König wrote:
[SNIP]
Imo the below semantics would be much cleaner:

- invalidate may add new fences
- invalidate _must_ unmap its mappings
- an unmap must wait for current fences before the mapping can be
released.

Imo there's no reason why unmap is special, and the only thing where we
don't use fences to gate access to resources/memory when it's in the
process of getting moved around.

Well in general I want to avoid waiting for fences as much as possible.
But the key point here is that this actually won't help with the problem
I'm trying to solve.

The point of using fences is not to wait on them. I mean if you have
the shadow ttm bo on the lru you also don't wait for that fence to
retire before you insert the shadow bo onto the lru. You don't even
wait when you try to use that memory again, you just pipeline more
stuff on top.


Correct.

Ok, if I understand it correctly your suggestion is to move the 
responsibility to delay destruction of mappings until they are no longer 
used from the importer to the exporter based on the fences of the 
reservation object.


I seriously don't think that this is a good idea because you need to 
move the tracking who is using which mapping from the importer to the 
exporter as well. So duplicating quite a bunch of housekeeping.


On the other hand that we have this house keeping in the importer 
because we get it for free from TTM. But I can't think of a way other 
memory management backends would do this without keeping the sg table 
around either.



In the end it will be the exact same amount of fences and waiting in
both solutions. One just leaks less implementationt details (at least
in my opinion) across the dma-buf border.


I agree that leaking implementation details over the DMA-buf border is a 
bad idea.


But I can assure you that this has absolutely nothing todo with the 
ghost object handling of TTM. ghost objects doesn't even receive an 
invalidation, they are just a possible implementation of the delayed 
destruction of sg tables.



btw this is like the 2nd or 3rd time I'm typing this, haven't seen your
thoughts on this yet.

Yeah, and I'm responding for the 3rd time now that you are
misunderstanding why we need this here :)

Maybe I can make that clear with an example:

1. You got a sharing between device A (exporter) and B (importer) which
uses P2P.

2. Now device C (importer) comes along and wants to use the DMA-buf
object as well.

3. The handling now figures out that we can't do P2P between device A
and device C (for whatever reason).

4. The map_attachment implementation in device driver A doesn't want to
fail with -EBUSY and migrates the DMA-buf somewhere where both device A
and device C can access it.

5. This migration will result in sending an invalidation event around.
And here it doesn't make sense to send this invalidation event to device
C, because we know that device C is actually causing this event and
doesn't have a valid mapping.

Hm I thought the last time around there was a different scenario, with
just one importer:

- importer has a mapping, gets an ->invalidate call.
- importer arranges for the mappings/usage to get torn down, maybe
updating fences, all from ->invalidate. But the mapping itself wont
disappear.
- exporter moves buffer to new places (for whatever reasons it felt
that was the thing to do).
- importer does another execbuf, the exporter needs to move the buffer
back. Again it calls ->invalidate, but on a mapping it already has
called ->invalidate on, and to prevent that silliness we take the
importer temporary off the list.


Mhm, strange I don't remember giving this explanation. It also doesn't 
make to much sense, but see below.



Your scenario here is new, and iirc my suggestion back then was to
count the number of pending mappings so you don't go around calling
->invalidate on mappings that don't exist.


Well the key point is we don't call invalidate on mappings, but we call 
invalidate on attachments.


When the invalidate on an attachment is received all the importer should 
at least start to tear down all mappings.



But even if you fix your scenario here there's still the issue that we
can receive invalidates on a mapping we've already torn down and which
is on the process of disappearing. That's kinda the part I don't think
is great semantics.


Yeah, that is a rather valid point.

Currently it is perfectly valid to receive an invalidation when you 
don't even have any mappings at all.


But this is intentional, because otherwise I would need to move the 
housekeeping which mappings are currently made from the importer to the 
exporter.


And as explained above that would essentially mean double housekeeping.


[SNIP]

The reason I don't have that on unmap is that I think migrating things
on unmap doesn'

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/21, Daniel Vetter wrote:
> On Fri, Jun 21, 2019 at 1:50 PM Daniel Vetter  wrote:
> >
> > On Fri, Jun 21, 2019 at 1:37 PM Christian König
> >  wrote:
> > >
> > > Am 21.06.19 um 13:03 schrieb Daniel Vetter:
> > > > On Fri, Jun 21, 2019 at 12:31 PM Koenig, Christian
> > > >  wrote:
> > > >> Am 21.06.19 um 12:20 schrieb Emil Velikov:
> > > >>> In particular, that user-space will "remove" render nodes.
> > > >> Yes, that is my main concern here. You basically make render nodes
> > > >> superfluously. That gives userspace all legitimization to also remove
> > > >> support for them. That is not stupid or evil or whatever, userspace
> > > >> would just follow the kernel design.
> > > > This already happened. At least for kms-only display socs we had to
> > > > hide the separate render node (and there you really have to deal with
> > > > the separate render node, because it's a distinct driver) entirely
> > > > within gbm/mesa.
> > >
> > > Ok, that is something I didn't knew before and is rather interesting.
> > >
> > > > So if you want to depracate render functionality on primary nodes, you
> > > > _have_ to do that hiding in userspace. Or you'll break a lot of
> > > > compositors everywhere. Just testing -amdgpu doesn't really prove
> > > > anything here. So you won't move the larger ecosystem with this at
> > > > all, that ship sailed.
> > >
> > > So what else can we do? That sounds like you suggest we should
> > > completely drop render node functionality.
> > >
> > > I mean it's not my preferred option, but certainly something that
> > > everybody could do.
> > >
> > > My primary concern is that we somehow need to get rid of thinks like GEM
> > > flink and all that other crufty stuff we still have around on the
> > > primary node (we should probably make a list of that).
> > >
> > > Switching everything over to render nodes just sounded like the best
> > > alternative so far to archive that.
> >
> > tbh I do like that plan too, but it's a lot more work. And I think to
> > have any push whatsoever we probably need to roll it out in gbm as a
> > hack to keep things going. But probably not enough.
> >
> > I also think that at least some compositors will bother to do the
> > right thing, and actually bother with render nodes and all that
> > correctly. It's just that there's way more which dont.
> >
> > Also for server rendering it'll be render nodes all the way down I
> > hope (or we need to seriously educate cloud people about the
> > permissions they set on their default images, and distros on how this
> > cloud stuff should work.
> >
> > > > At that point this all feels like a bikeshed, and for a bikeshed I
> > > > don't care what the color is we pick, as long as they're all painted
> > > > the same.
> > > >
> > > > Once we picked a color it's a simple technical matter of how to roll
> > > > it out, using Kconfig options, or only enabling on new hw, or "merge
> > > > and fix the regressions as they come" or any of the other well proven
> > > > paths forward.
> > >
> > > Yeah, the problem is I don't see an option which currently works for
> > > everyone.
> > >
> > > I absolutely need a grace time of multiple years until we can apply this
> > > to amdgpu/radeon.
> >
> > Yeah that's what I meant with "pick a color, pick a way to roll it
> > out". "enable for new hw, roll out years and years later" is one of
> > the options for roll out.
> >
> > > And that under the prerequisite to have a plan to somehow enable that
> > > functionality now to make it at least painful for userspace to rely on
> > > hack around that.
> > >
> > > > So if you want to do this, please start with the mesa side work (as
> > > > the biggest userspace, not all of it) with patches to redirect all
> > > > primary node rendering to render nodes. The code is there already for
> > > > socs, just needs to be rolled out more. Or we go with the DRM_AUTH
> > > > horrors. Or a 3rd option, I really don't care which it is, as long as
> > > > its consistent.
> > >
> > > How about this:
> > > 1. We keep DRM_AUTH around for amdgpu/radeon for now.
> > > 2. We add a Kconfig option to ignore DRM_AUTH, currently default to N.
> > > This also works as a workaround for old RADV.
> > > 3. We start to work on further removing old cruft from the primary node.
> > > Possible the Kconfig option I suggested to disable GEM flink.
> >
> > Hm I'm not worried about flink really. It's gem_open which is the
> > security gap, and that one has to stay master-only forever. I guess we
> > could try to disable that so people have to deal with dma-buf (and
> > once you have that render nodes become a lot easier to use). But then
> > I still think we have drivers which don't do dma-buf self-import, so
> > again we're stuck. So maybe first step is to just roll out a default
> > self-import dma-buf support for every gem driver. Then start ditching
> > flink/gem_open. Then start ditching render support on primary nodes.
> > Every step in the way taking a few years of prod

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/21, Koenig, Christian wrote:
> Am 21.06.19 um 12:53 schrieb Emil Velikov:
> > On 2019/06/21, Koenig, Christian wrote:
> >> [SNIP]
> >> Well partially. That RADV broke is unfortunate, but we have done so many
> >> customized userspace stuff both based on Mesa/DDX as well as closed
> >> source components that it is just highly likely that we would break
> >> something else as well.
> >>
> > As an engineer I like to work with tangibles. The highly likely part is
> > probable, but as mentioned multiple times the series allows for a _dead_
> > trivial way to address any such problems.
> 
> Well to clarify my concern is that this won't be so trivial.
> 
> You implemented on workaround for one specific case and it is perfectly 
> possible that for other cases we would have to completely revert the 
> removal of DRM_AUTH.
> 
I would encourage you to take a closer look at my patch and point out
how parcicular cases cannot be handled.

> >>> In particular, that user-space will "remove" render nodes.
> >> Yes, that is my main concern here. You basically make render nodes
> >> superfluously. That gives userspace all legitimization to also remove
> >> support for them. That is not stupid or evil or whatever, userspace
> >> would just follow the kernel design.
> >>
> > Do you have an example of userspace doing such an extremely silly thing?
> > It does seem like suspect you're a couple of steps beyond overcautious,
> > perhaps rightfully so. Maybe you've seen some closed-source user-space
> > going crazy? Or any other projects?
> 
> The key point is that I don't think this is silly or strange or crazy at 
> all. See the kernel defines the interface userspace can and should use.
> 
> When the kernel defines that everything will work with the primary node 
> it is perfectly valid for userspace to drop support for the render node.
> 
> I mean why should they keep this? Just because we tell them not to do this?
> 
From your experiense, do user-space developers care about doing (or
generally do) the right thing?

In either case, as pointed already the cat is out of the bag - has been
for years, and if developers did behave as you describe them they would
have "removed" render nodes already.

> > In other words, being cautious is great, but without references of
> > misuse it's very hard for others to draw the full picture.
> >
> >>> I'm really sad that amdgpu insists on standing out, hope one day it will
> >>> converge. Yet since all other maintainers are ok with the series, I'll
> >>> start merging patches in a few hours. I'm really sad that amdgpu wants
> >>> to stand out, hope it will converge sooner rather than later.
> >>>
> >>> Christian, how would you like amdgpu handled - with a separate flag in
> >>> the driver or shall we special case amdgpu within core drm?
> >> No, I ask you to please stick around DRM_AUTH for at least amdgpu and
> >> radeon. And NOT enable any render node functionality for them on the
> >> primary node.
> >>
> > My question is how do you want this handled:
> >   - DRM_PLEASE_FORCE_AUTH - added to AMDGPU/RADEON, or
> >   - driver_name == amdgpu, in core DRM.
> 
> I want to keep DRM_AUTH in amdgpu and radeon for at least the next 5-10 
> years.
> 
Believe we're all fully aware of that fact. I'm asking which _approach_
you prefer. That said, I'll send a new patch/series and we'll nitpick it
there.

Thanks
-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Daniel Vetter]

On Fri, Jun 21, 2019 at 1:50 PM Daniel Vetter  wrote:
>
> On Fri, Jun 21, 2019 at 1:37 PM Christian König
>  wrote:
> >
> > Am 21.06.19 um 13:03 schrieb Daniel Vetter:
> > > On Fri, Jun 21, 2019 at 12:31 PM Koenig, Christian
> > >  wrote:
> > >> Am 21.06.19 um 12:20 schrieb Emil Velikov:
> > >>> In particular, that user-space will "remove" render nodes.
> > >> Yes, that is my main concern here. You basically make render nodes
> > >> superfluously. That gives userspace all legitimization to also remove
> > >> support for them. That is not stupid or evil or whatever, userspace
> > >> would just follow the kernel design.
> > > This already happened. At least for kms-only display socs we had to
> > > hide the separate render node (and there you really have to deal with
> > > the separate render node, because it's a distinct driver) entirely
> > > within gbm/mesa.
> >
> > Ok, that is something I didn't knew before and is rather interesting.
> >
> > > So if you want to depracate render functionality on primary nodes, you
> > > _have_ to do that hiding in userspace. Or you'll break a lot of
> > > compositors everywhere. Just testing -amdgpu doesn't really prove
> > > anything here. So you won't move the larger ecosystem with this at
> > > all, that ship sailed.
> >
> > So what else can we do? That sounds like you suggest we should
> > completely drop render node functionality.
> >
> > I mean it's not my preferred option, but certainly something that
> > everybody could do.
> >
> > My primary concern is that we somehow need to get rid of thinks like GEM
> > flink and all that other crufty stuff we still have around on the
> > primary node (we should probably make a list of that).
> >
> > Switching everything over to render nodes just sounded like the best
> > alternative so far to archive that.
>
> tbh I do like that plan too, but it's a lot more work. And I think to
> have any push whatsoever we probably need to roll it out in gbm as a
> hack to keep things going. But probably not enough.
>
> I also think that at least some compositors will bother to do the
> right thing, and actually bother with render nodes and all that
> correctly. It's just that there's way more which dont.
>
> Also for server rendering it'll be render nodes all the way down I
> hope (or we need to seriously educate cloud people about the
> permissions they set on their default images, and distros on how this
> cloud stuff should work.
>
> > > At that point this all feels like a bikeshed, and for a bikeshed I
> > > don't care what the color is we pick, as long as they're all painted
> > > the same.
> > >
> > > Once we picked a color it's a simple technical matter of how to roll
> > > it out, using Kconfig options, or only enabling on new hw, or "merge
> > > and fix the regressions as they come" or any of the other well proven
> > > paths forward.
> >
> > Yeah, the problem is I don't see an option which currently works for
> > everyone.
> >
> > I absolutely need a grace time of multiple years until we can apply this
> > to amdgpu/radeon.
>
> Yeah that's what I meant with "pick a color, pick a way to roll it
> out". "enable for new hw, roll out years and years later" is one of
> the options for roll out.
>
> > And that under the prerequisite to have a plan to somehow enable that
> > functionality now to make it at least painful for userspace to rely on
> > hack around that.
> >
> > > So if you want to do this, please start with the mesa side work (as
> > > the biggest userspace, not all of it) with patches to redirect all
> > > primary node rendering to render nodes. The code is there already for
> > > socs, just needs to be rolled out more. Or we go with the DRM_AUTH
> > > horrors. Or a 3rd option, I really don't care which it is, as long as
> > > its consistent.
> >
> > How about this:
> > 1. We keep DRM_AUTH around for amdgpu/radeon for now.
> > 2. We add a Kconfig option to ignore DRM_AUTH, currently default to N.
> > This also works as a workaround for old RADV.
> > 3. We start to work on further removing old cruft from the primary node.
> > Possible the Kconfig option I suggested to disable GEM flink.
>
> Hm I'm not worried about flink really. It's gem_open which is the
> security gap, and that one has to stay master-only forever. I guess we
> could try to disable that so people have to deal with dma-buf (and
> once you have that render nodes become a lot easier to use). But then
> I still think we have drivers which don't do dma-buf self-import, so
> again we're stuck. So maybe first step is to just roll out a default
> self-import dma-buf support for every gem driver. Then start ditching
> flink/gem_open. Then start ditching render support on primary nodes.
> Every step in the way taking a few years of prodding userspace plus
> even more years to wait until it's all gone.

I forgot one step here: I think we even still have drivers without
render node support. As long as those exists (and are still relevant)
then userspace needs primary 

[PATCH v2 14/18] drm/amdgpu: switch driver from bo->resv to bo->base.resv

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  6 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c|  6 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c   |  4 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c   |  6 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c|  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c| 20 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c   |  6 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c   |  4 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c| 30 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c   |  2 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +-
 13 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index df26bf34b675..6dce43bd60f1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -218,7 +218,7 @@ void amdgpu_amdkfd_unreserve_memory_limit(struct amdgpu_bo 
*bo)
 static int amdgpu_amdkfd_remove_eviction_fence(struct amdgpu_bo *bo,
struct amdgpu_amdkfd_fence *ef)
 {
-   struct reservation_object *resv = bo->tbo.resv;
+   struct reservation_object *resv = bo->tbo.base.resv;
struct reservation_object_list *old, *new;
unsigned int i, j, k;
 
@@ -812,7 +812,7 @@ static int process_sync_pds_resv(struct amdkfd_process_info 
*process_info,
struct amdgpu_bo *pd = peer_vm->root.base.bo;
 
ret = amdgpu_sync_resv(NULL,
-   sync, pd->tbo.resv,
+   sync, pd->tbo.base.resv,
AMDGPU_FENCE_OWNER_UNDEFINED, false);
if (ret)
return ret;
@@ -887,7 +887,7 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void 
**process_info,
  AMDGPU_FENCE_OWNER_KFD, false);
if (ret)
goto wait_pd_fail;
-   ret = reservation_object_reserve_shared(vm->root.base.bo->tbo.resv, 1);
+   ret = 
reservation_object_reserve_shared(vm->root.base.bo->tbo.base.resv, 1);
if (ret)
goto reserve_shared_fail;
amdgpu_bo_fence(vm->root.base.bo,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index dc63707e426f..118ec7514277 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -402,7 +402,7 @@ static int amdgpu_cs_bo_validate(struct amdgpu_cs_parser *p,
struct ttm_operation_ctx ctx = {
.interruptible = true,
.no_wait_gpu = false,
-   .resv = bo->tbo.resv,
+   .resv = bo->tbo.base.resv,
.flags = 0
};
uint32_t domain;
@@ -734,7 +734,7 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
 
list_for_each_entry(e, &p->validated, tv.head) {
struct amdgpu_bo *bo = ttm_to_amdgpu_bo(e->tv.bo);
-   struct reservation_object *resv = bo->tbo.resv;
+   struct reservation_object *resv = bo->tbo.base.resv;
 
r = amdgpu_sync_resv(p->adev, &p->job->sync, resv, p->filp,
 amdgpu_bo_explicit_sync(bo));
@@ -1732,7 +1732,7 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser 
*parser,
*map = mapping;
 
/* Double check that the BO is reserved by this CS */
-   if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
+   if (READ_ONCE((*bo)->tbo.base.resv->lock.ctx) != &parser->ticket)
return -EINVAL;
 
if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 535650967b1a..b5d020e15c35 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -204,7 +204,7 @@ int amdgpu_display_crtc_page_flip_target(struct drm_crtc 
*crtc,
goto unpin;
}
 
-   r = reservation_object_get_fences_rcu(new_abo->tbo.resv, &work->excl,
+   r = reservation_object_get_fences_rcu(new_abo->tbo.base.resv, 
&work->excl,
  &work->shared_count,
  &work->shared);
if (unlikely(r != 0)) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index c56819bcad8e..cd4fe9dd8863 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -216,7 +216,7 @@ static int amdgpu_dma_buf_map_attach(struct dma_buf 
*dma_buf,
 * fences on the reser

[PATCH v2 08/18] drm/ttm: use gem vma_node

[Gerd Hoffmann]

Drop vma_node from ttm_buffer_object, use the gem struct
(base.vma_node) instead.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 2 +-
 drivers/gpu/drm/qxl/qxl_object.h   | 2 +-
 drivers/gpu/drm/radeon/radeon_object.h | 2 +-
 drivers/gpu/drm/virtio/virtgpu_drv.h   | 2 +-
 include/drm/ttm/ttm_bo_api.h   | 5 +
 drivers/gpu/drm/drm_gem_vram_helper.c  | 5 +
 drivers/gpu/drm/nouveau/nouveau_display.c  | 2 +-
 drivers/gpu/drm/nouveau/nouveau_gem.c  | 2 +-
 drivers/gpu/drm/ttm/ttm_bo.c   | 8 
 drivers/gpu/drm/ttm/ttm_bo_util.c  | 2 +-
 drivers/gpu/drm/ttm/ttm_bo_vm.c| 9 +
 drivers/gpu/drm/virtio/virtgpu_prime.c | 3 ---
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 4 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c| 4 ++--
 14 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index a80a9972ad16..a68d85bd8fab 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -191,7 +191,7 @@ static inline unsigned amdgpu_bo_gpu_page_alignment(struct 
amdgpu_bo *bo)
  */
 static inline u64 amdgpu_bo_mmap_offset(struct amdgpu_bo *bo)
 {
-   return drm_vma_node_offset_addr(&bo->tbo.vma_node);
+   return drm_vma_node_offset_addr(&bo->tbo.base.vma_node);
 }
 
 /**
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index b812d4ae9d0d..8ae54ba7857c 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -60,7 +60,7 @@ static inline unsigned long qxl_bo_size(struct qxl_bo *bo)
 
 static inline u64 qxl_bo_mmap_offset(struct qxl_bo *bo)
 {
-   return drm_vma_node_offset_addr(&bo->tbo.vma_node);
+   return drm_vma_node_offset_addr(&bo->tbo.base.vma_node);
 }
 
 static inline int qxl_bo_wait(struct qxl_bo *bo, u32 *mem_type,
diff --git a/drivers/gpu/drm/radeon/radeon_object.h 
b/drivers/gpu/drm/radeon/radeon_object.h
index 9ffd8215d38a..e5554bf9140e 100644
--- a/drivers/gpu/drm/radeon/radeon_object.h
+++ b/drivers/gpu/drm/radeon/radeon_object.h
@@ -116,7 +116,7 @@ static inline unsigned radeon_bo_gpu_page_alignment(struct 
radeon_bo *bo)
  */
 static inline u64 radeon_bo_mmap_offset(struct radeon_bo *bo)
 {
-   return drm_vma_node_offset_addr(&bo->tbo.vma_node);
+   return drm_vma_node_offset_addr(&bo->tbo.base.vma_node);
 }
 
 extern int radeon_bo_wait(struct radeon_bo *bo, u32 *mem_type,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 9e2d3062b01d..7146ba00fd5b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -396,7 +396,7 @@ static inline void virtio_gpu_object_unref(struct 
virtio_gpu_object **bo)
 
 static inline u64 virtio_gpu_object_mmap_offset(struct virtio_gpu_object *bo)
 {
-   return drm_vma_node_offset_addr(&bo->tbo.vma_node);
+   return drm_vma_node_offset_addr(&bo->tbo.base.vma_node);
 }
 
 static inline int virtio_gpu_object_reserve(struct virtio_gpu_object *bo,
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 88aa7bf1b18a..77bd420a147a 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -152,7 +152,6 @@ struct ttm_tt;
  * @ddestroy: List head for the delayed destroy list.
  * @swap: List head for swap LRU list.
  * @moving: Fence set when BO is moving
- * @vma_node: Address space manager node.
  * @offset: The current GPU offset, which can have different meanings
  * depending on the memory type. For SYSTEM type memory, it should be 0.
  * @cur_placement: Hint of current placement.
@@ -219,9 +218,7 @@ struct ttm_buffer_object {
 */
 
struct dma_fence *moving;
-
-   struct drm_vma_offset_node vma_node;
-
+   /* base.vma_node */
unsigned priority;
 
/**
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c 
b/drivers/gpu/drm/drm_gem_vram_helper.c
index 61d9520cc15f..2e474dee30df 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -163,7 +163,7 @@ EXPORT_SYMBOL(drm_gem_vram_put);
  */
 u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo)
 {
-   return drm_vma_node_offset_addr(&gbo->bo.vma_node);
+   return drm_vma_node_offset_addr(&gbo->bo.base.vma_node);
 }
 EXPORT_SYMBOL(drm_gem_vram_mmap_offset);
 
@@ -633,9 +633,6 @@ EXPORT_SYMBOL(drm_gem_vram_driver_gem_prime_vunmap);
 int drm_gem_vram_driver_gem_prime_mmap(struct drm_gem_object *gem,
   struct vm_area_struct *vma)
 {
-   struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
-
-   gbo->bo.base.vma_node.vm_node.start = gbo->bo.vma_node.vm_node.start;
return drm_gem_prime_mmap(gem, vma);
 }
 EXPORT_SYMBOL(drm_gem_vram_driver_gem_prime_mmap);
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c 

[PATCH v2 12/18] drm/radeon: switch driver from bo->resv to bo->base.resv

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/radeon/radeon_benchmark.c | 4 ++--
 drivers/gpu/drm/radeon/radeon_cs.c| 2 +-
 drivers/gpu/drm/radeon/radeon_display.c   | 2 +-
 drivers/gpu/drm/radeon/radeon_gem.c   | 6 +++---
 drivers/gpu/drm/radeon/radeon_mn.c| 2 +-
 drivers/gpu/drm/radeon/radeon_object.c| 8 
 drivers/gpu/drm/radeon/radeon_prime.c | 2 +-
 drivers/gpu/drm/radeon/radeon_test.c  | 8 
 drivers/gpu/drm/radeon/radeon_ttm.c   | 2 +-
 drivers/gpu/drm/radeon/radeon_uvd.c   | 2 +-
 drivers/gpu/drm/radeon/radeon_vm.c| 6 +++---
 11 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_benchmark.c 
b/drivers/gpu/drm/radeon/radeon_benchmark.c
index 7ce5064a59f6..1ea50ce16312 100644
--- a/drivers/gpu/drm/radeon/radeon_benchmark.c
+++ b/drivers/gpu/drm/radeon/radeon_benchmark.c
@@ -122,7 +122,7 @@ static void radeon_benchmark_move(struct radeon_device 
*rdev, unsigned size,
if (rdev->asic->copy.dma) {
time = radeon_benchmark_do_move(rdev, size, saddr, daddr,
RADEON_BENCHMARK_COPY_DMA, n,
-   dobj->tbo.resv);
+   dobj->tbo.base.resv);
if (time < 0)
goto out_cleanup;
if (time > 0)
@@ -133,7 +133,7 @@ static void radeon_benchmark_move(struct radeon_device 
*rdev, unsigned size,
if (rdev->asic->copy.blit) {
time = radeon_benchmark_do_move(rdev, size, saddr, daddr,
RADEON_BENCHMARK_COPY_BLIT, n,
-   dobj->tbo.resv);
+   dobj->tbo.base.resv);
if (time < 0)
goto out_cleanup;
if (time > 0)
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c 
b/drivers/gpu/drm/radeon/radeon_cs.c
index d206654b31ad..7e5254a34e84 100644
--- a/drivers/gpu/drm/radeon/radeon_cs.c
+++ b/drivers/gpu/drm/radeon/radeon_cs.c
@@ -257,7 +257,7 @@ static int radeon_cs_sync_rings(struct radeon_cs_parser *p)
list_for_each_entry(reloc, &p->validated, tv.head) {
struct reservation_object *resv;
 
-   resv = reloc->robj->tbo.resv;
+   resv = reloc->robj->tbo.base.resv;
r = radeon_sync_resv(p->rdev, &p->ib.sync, resv,
 reloc->tv.num_shared);
if (r)
diff --git a/drivers/gpu/drm/radeon/radeon_display.c 
b/drivers/gpu/drm/radeon/radeon_display.c
index ea6b752dd3a4..7bf73230ac0b 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -533,7 +533,7 @@ static int radeon_crtc_page_flip_target(struct drm_crtc 
*crtc,
DRM_ERROR("failed to pin new rbo buffer before flip\n");
goto cleanup;
}
-   work->fence = 
dma_fence_get(reservation_object_get_excl(new_rbo->tbo.resv));
+   work->fence = 
dma_fence_get(reservation_object_get_excl(new_rbo->tbo.base.resv));
radeon_bo_get_tiling_flags(new_rbo, &tiling_flags, NULL);
radeon_bo_unreserve(new_rbo);
 
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
b/drivers/gpu/drm/radeon/radeon_gem.c
index 7238007f5aa4..03873f21a734 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -114,7 +114,7 @@ static int radeon_gem_set_domain(struct drm_gem_object 
*gobj,
}
if (domain == RADEON_GEM_DOMAIN_CPU) {
/* Asking for cpu access wait for object idle */
-   r = reservation_object_wait_timeout_rcu(robj->tbo.resv, true, 
true, 30 * HZ);
+   r = reservation_object_wait_timeout_rcu(robj->tbo.base.resv, 
true, true, 30 * HZ);
if (!r)
r = -EBUSY;
 
@@ -449,7 +449,7 @@ int radeon_gem_busy_ioctl(struct drm_device *dev, void 
*data,
}
robj = gem_to_radeon_bo(gobj);
 
-   r = reservation_object_test_signaled_rcu(robj->tbo.resv, true);
+   r = reservation_object_test_signaled_rcu(robj->tbo.base.resv, true);
if (r == 0)
r = -EBUSY;
else
@@ -478,7 +478,7 @@ int radeon_gem_wait_idle_ioctl(struct drm_device *dev, void 
*data,
}
robj = gem_to_radeon_bo(gobj);
 
-   ret = reservation_object_wait_timeout_rcu(robj->tbo.resv, true, true, 
30 * HZ);
+   ret = reservation_object_wait_timeout_rcu(robj->tbo.base.resv, true, 
true, 30 * HZ);
if (ret == 0)
r = -EBUSY;
else if (ret < 0)
diff --git a/drivers/gpu/drm/radeon/radeon_mn.c 
b/drivers/gpu/drm/radeon/radeon_mn.c
index 8c3871ed23a9..0d64ace0e6c1 100644
--- a/drivers/gpu/drm/radeon/radeon_mn.c
+++ b/drivers/gpu/drm/radeon/radeon_mn.c
@@ -163,7 +163,7 @@ static int radeon_mn_invalidate_range_start(struct 
mmu_no

[PATCH v2 04/18] drm/radeon: use embedded gem object

[Gerd Hoffmann]

Drop drm_gem_object from radeon_bo, use the
ttm_buffer_object.base instead.

Build tested only.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/radeon/radeon.h |  3 +--
 drivers/gpu/drm/radeon/radeon_cs.c  |  2 +-
 drivers/gpu/drm/radeon/radeon_display.c |  4 ++--
 drivers/gpu/drm/radeon/radeon_gem.c |  2 +-
 drivers/gpu/drm/radeon/radeon_object.c  | 14 +++---
 drivers/gpu/drm/radeon/radeon_prime.c   |  2 +-
 drivers/gpu/drm/radeon/radeon_ttm.c |  2 +-
 7 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 32808e50be12..3f7701321d21 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -505,7 +505,6 @@ struct radeon_bo {
struct list_headva;
/* Constant after initialization */
struct radeon_device*rdev;
-   struct drm_gem_object   gem_base;
 
struct ttm_bo_kmap_obj  dma_buf_vmap;
pid_t   pid;
@@ -513,7 +512,7 @@ struct radeon_bo {
struct radeon_mn*mn;
struct list_headmn_list;
 };
-#define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, gem_base)
+#define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, tbo.base)
 
 int radeon_gem_debugfs_init(struct radeon_device *rdev);
 
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c 
b/drivers/gpu/drm/radeon/radeon_cs.c
index cef0e697a2ea..d206654b31ad 100644
--- a/drivers/gpu/drm/radeon/radeon_cs.c
+++ b/drivers/gpu/drm/radeon/radeon_cs.c
@@ -443,7 +443,7 @@ static void radeon_cs_parser_fini(struct radeon_cs_parser 
*parser, int error, bo
if (bo == NULL)
continue;
 
-   drm_gem_object_put_unlocked(&bo->gem_base);
+   drm_gem_object_put_unlocked(&bo->tbo.base);
}
}
kfree(parser->track);
diff --git a/drivers/gpu/drm/radeon/radeon_display.c 
b/drivers/gpu/drm/radeon/radeon_display.c
index bd52f15e6330..ea6b752dd3a4 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -275,7 +275,7 @@ static void radeon_unpin_work_func(struct work_struct 
*__work)
} else
DRM_ERROR("failed to reserve buffer after flip\n");
 
-   drm_gem_object_put_unlocked(&work->old_rbo->gem_base);
+   drm_gem_object_put_unlocked(&work->old_rbo->tbo.base);
kfree(work);
 }
 
@@ -607,7 +607,7 @@ static int radeon_crtc_page_flip_target(struct drm_crtc 
*crtc,
radeon_bo_unreserve(new_rbo);
 
 cleanup:
-   drm_gem_object_put_unlocked(&work->old_rbo->gem_base);
+   drm_gem_object_put_unlocked(&work->old_rbo->tbo.base);
dma_fence_put(work->fence);
kfree(work);
return r;
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
b/drivers/gpu/drm/radeon/radeon_gem.c
index d8bc5d2dfd61..7238007f5aa4 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -83,7 +83,7 @@ int radeon_gem_object_create(struct radeon_device *rdev, 
unsigned long size,
}
return r;
}
-   *obj = &robj->gem_base;
+   *obj = &robj->tbo.base;
robj->pid = task_pid_nr(current);
 
mutex_lock(&rdev->gem.mutex);
diff --git a/drivers/gpu/drm/radeon/radeon_object.c 
b/drivers/gpu/drm/radeon/radeon_object.c
index 21f73fc86f38..d96c2cb7d584 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -85,9 +85,9 @@ static void radeon_ttm_bo_destroy(struct ttm_buffer_object 
*tbo)
mutex_unlock(&bo->rdev->gem.mutex);
radeon_bo_clear_surface_reg(bo);
WARN_ON_ONCE(!list_empty(&bo->va));
-   if (bo->gem_base.import_attach)
-   drm_prime_gem_destroy(&bo->gem_base, bo->tbo.sg);
-   drm_gem_object_release(&bo->gem_base);
+   if (bo->tbo.base.import_attach)
+   drm_prime_gem_destroy(&bo->tbo.base, bo->tbo.sg);
+   drm_gem_object_release(&bo->tbo.base);
kfree(bo);
 }
 
@@ -209,7 +209,7 @@ int radeon_bo_create(struct radeon_device *rdev,
bo = kzalloc(sizeof(struct radeon_bo), GFP_KERNEL);
if (bo == NULL)
return -ENOMEM;
-   drm_gem_private_object_init(rdev->ddev, &bo->gem_base, size);
+   drm_gem_private_object_init(rdev->ddev, &bo->tbo.base, size);
bo->rdev = rdev;
bo->surface_reg = -1;
INIT_LIST_HEAD(&bo->list);
@@ -442,13 +442,13 @@ void radeon_bo_force_delete(struct radeon_device *rdev)
dev_err(rdev->dev, "Userspace still has active objects !\n");
list_for_each_entry_safe(bo, n, &rdev->gem.objects, list) {
dev_err(rdev->dev, "%p %p %lu %lu force free\n",
-   &bo->gem_base, bo, (unsigned long)bo->gem_base.size,
-   *((unsigned long *)&bo->gem_base

[PATCH v2 05/18] drm/amdgpu: use embedded gem object

[Gerd Hoffmann]

Drop drm_gem_object from amdgpu_bo, use the
ttm_buffer_object.base instead.

Build tested only.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h  | 1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 8 
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 8 
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 2 +-
 6 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
index b8ba6e27c61f..2f17150e26e1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
@@ -31,7 +31,7 @@
  */
 
 #define AMDGPU_GEM_DOMAIN_MAX  0x3
-#define gem_to_amdgpu_bo(gobj) container_of((gobj), struct amdgpu_bo, gem_base)
+#define gem_to_amdgpu_bo(gobj) container_of((gobj), struct amdgpu_bo, tbo.base)
 
 void amdgpu_gem_object_free(struct drm_gem_object *obj);
 int amdgpu_gem_object_open(struct drm_gem_object *obj,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index c430e8259038..a80a9972ad16 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -94,7 +94,6 @@ struct amdgpu_bo {
/* per VM structure for page tables and with virtual addresses */
struct amdgpu_vm_bo_base*vm_bo;
/* Constant after initialization */
-   struct drm_gem_object   gem_base;
struct amdgpu_bo*parent;
struct amdgpu_bo*shadow;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 489041df1f45..c56819bcad8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -409,7 +409,7 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
bo->prime_shared_count = 1;
 
ww_mutex_unlock(&resv->lock);
-   return &bo->gem_base;
+   return &bo->tbo.base;
 
 error:
ww_mutex_unlock(&resv->lock);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 37b526c6f494..6d991e8df357 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -85,7 +85,7 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, 
unsigned long size,
}
return r;
}
-   *obj = &bo->gem_base;
+   *obj = &bo->tbo.base;
 
return 0;
 }
@@ -690,7 +690,7 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
struct drm_amdgpu_gem_create_in info;
void __user *out = u64_to_user_ptr(args->value);
 
-   info.bo_size = robj->gem_base.size;
+   info.bo_size = robj->tbo.base.size;
info.alignment = robj->tbo.mem.page_alignment << PAGE_SHIFT;
info.domains = robj->preferred_domains;
info.domain_flags = robj->flags;
@@ -820,8 +820,8 @@ static int amdgpu_debugfs_gem_bo_info(int id, void *ptr, 
void *data)
if (pin_count)
seq_printf(m, " pin count %d", pin_count);
 
-   dma_buf = READ_ONCE(bo->gem_base.dma_buf);
-   attachment = READ_ONCE(bo->gem_base.import_attach);
+   dma_buf = READ_ONCE(bo->tbo.base.dma_buf);
+   attachment = READ_ONCE(bo->tbo.base.import_attach);
 
if (attachment)
seq_printf(m, " imported from %p", dma_buf);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 16f96f2e3671..00b283f914a6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -85,9 +85,9 @@ static void amdgpu_bo_destroy(struct ttm_buffer_object *tbo)
 
amdgpu_bo_kunmap(bo);
 
-   if (bo->gem_base.import_attach)
-   drm_prime_gem_destroy(&bo->gem_base, bo->tbo.sg);
-   drm_gem_object_release(&bo->gem_base);
+   if (bo->tbo.base.import_attach)
+   drm_prime_gem_destroy(&bo->tbo.base, bo->tbo.sg);
+   drm_gem_object_release(&bo->tbo.base);
/* in case amdgpu_device_recover_vram got NULL of bo->parent */
if (!list_empty(&bo->shadow_list)) {
mutex_lock(&adev->shadow_list_lock);
@@ -454,7 +454,7 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
bo = kzalloc(sizeof(struct amdgpu_bo), GFP_KERNEL);
if (bo == NULL)
return -ENOMEM;
-   drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
+   drm_gem_private_object_init(adev->ddev, &bo->tbo.base, size);
INIT_LIST_HEAD(&bo->shadow_list);
bo->vm_bo = NULL;
bo->preferred_domains = bp->preferred_domain ? bp->preferred_domain :
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/dr

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Daniel Vetter]

On Fri, Jun 21, 2019 at 1:37 PM Christian König
 wrote:
>
> Am 21.06.19 um 13:03 schrieb Daniel Vetter:
> > On Fri, Jun 21, 2019 at 12:31 PM Koenig, Christian
> >  wrote:
> >> Am 21.06.19 um 12:20 schrieb Emil Velikov:
> >>> In particular, that user-space will "remove" render nodes.
> >> Yes, that is my main concern here. You basically make render nodes
> >> superfluously. That gives userspace all legitimization to also remove
> >> support for them. That is not stupid or evil or whatever, userspace
> >> would just follow the kernel design.
> > This already happened. At least for kms-only display socs we had to
> > hide the separate render node (and there you really have to deal with
> > the separate render node, because it's a distinct driver) entirely
> > within gbm/mesa.
>
> Ok, that is something I didn't knew before and is rather interesting.
>
> > So if you want to depracate render functionality on primary nodes, you
> > _have_ to do that hiding in userspace. Or you'll break a lot of
> > compositors everywhere. Just testing -amdgpu doesn't really prove
> > anything here. So you won't move the larger ecosystem with this at
> > all, that ship sailed.
>
> So what else can we do? That sounds like you suggest we should
> completely drop render node functionality.
>
> I mean it's not my preferred option, but certainly something that
> everybody could do.
>
> My primary concern is that we somehow need to get rid of thinks like GEM
> flink and all that other crufty stuff we still have around on the
> primary node (we should probably make a list of that).
>
> Switching everything over to render nodes just sounded like the best
> alternative so far to archive that.

tbh I do like that plan too, but it's a lot more work. And I think to
have any push whatsoever we probably need to roll it out in gbm as a
hack to keep things going. But probably not enough.

I also think that at least some compositors will bother to do the
right thing, and actually bother with render nodes and all that
correctly. It's just that there's way more which dont.

Also for server rendering it'll be render nodes all the way down I
hope (or we need to seriously educate cloud people about the
permissions they set on their default images, and distros on how this
cloud stuff should work.

> > At that point this all feels like a bikeshed, and for a bikeshed I
> > don't care what the color is we pick, as long as they're all painted
> > the same.
> >
> > Once we picked a color it's a simple technical matter of how to roll
> > it out, using Kconfig options, or only enabling on new hw, or "merge
> > and fix the regressions as they come" or any of the other well proven
> > paths forward.
>
> Yeah, the problem is I don't see an option which currently works for
> everyone.
>
> I absolutely need a grace time of multiple years until we can apply this
> to amdgpu/radeon.

Yeah that's what I meant with "pick a color, pick a way to roll it
out". "enable for new hw, roll out years and years later" is one of
the options for roll out.

> And that under the prerequisite to have a plan to somehow enable that
> functionality now to make it at least painful for userspace to rely on
> hack around that.
>
> > So if you want to do this, please start with the mesa side work (as
> > the biggest userspace, not all of it) with patches to redirect all
> > primary node rendering to render nodes. The code is there already for
> > socs, just needs to be rolled out more. Or we go with the DRM_AUTH
> > horrors. Or a 3rd option, I really don't care which it is, as long as
> > its consistent.
>
> How about this:
> 1. We keep DRM_AUTH around for amdgpu/radeon for now.
> 2. We add a Kconfig option to ignore DRM_AUTH, currently default to N.
> This also works as a workaround for old RADV.
> 3. We start to work on further removing old cruft from the primary node.
> Possible the Kconfig option I suggested to disable GEM flink.

Hm I'm not worried about flink really. It's gem_open which is the
security gap, and that one has to stay master-only forever. I guess we
could try to disable that so people have to deal with dma-buf (and
once you have that render nodes become a lot easier to use). But then
I still think we have drivers which don't do dma-buf self-import, so
again we're stuck. So maybe first step is to just roll out a default
self-import dma-buf support for every gem driver. Then start ditching
flink/gem_open. Then start ditching render support on primary nodes.
Every step in the way taking a few years of prodding userspace plus
even more years to wait until it's all gone.

Another option would be to extract the kms stuff from primary nodes,
but we've tried that with control nodes. Until I realized a few years
back that with control nodes it's impossible to get any rendered
buffer in there at all, so useless, and I removed it. No one ever
complained.

So yeah would be really nice if we could fix this, but the universe
conspires against us too much it seems. Hence the fallback o

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Christian König]

Am 21.06.19 um 13:03 schrieb Daniel Vetter:

On Fri, Jun 21, 2019 at 12:31 PM Koenig, Christian
 wrote:

Am 21.06.19 um 12:20 schrieb Emil Velikov:

In particular, that user-space will "remove" render nodes.

Yes, that is my main concern here. You basically make render nodes
superfluously. That gives userspace all legitimization to also remove
support for them. That is not stupid or evil or whatever, userspace
would just follow the kernel design.

This already happened. At least for kms-only display socs we had to
hide the separate render node (and there you really have to deal with
the separate render node, because it's a distinct driver) entirely
within gbm/mesa.


Ok, that is something I didn't knew before and is rather interesting.


So if you want to depracate render functionality on primary nodes, you
_have_ to do that hiding in userspace. Or you'll break a lot of
compositors everywhere. Just testing -amdgpu doesn't really prove
anything here. So you won't move the larger ecosystem with this at
all, that ship sailed.


So what else can we do? That sounds like you suggest we should 
completely drop render node functionality.


I mean it's not my preferred option, but certainly something that 
everybody could do.


My primary concern is that we somehow need to get rid of thinks like GEM 
flink and all that other crufty stuff we still have around on the 
primary node (we should probably make a list of that).


Switching everything over to render nodes just sounded like the best 
alternative so far to archive that.



At that point this all feels like a bikeshed, and for a bikeshed I
don't care what the color is we pick, as long as they're all painted
the same.

Once we picked a color it's a simple technical matter of how to roll
it out, using Kconfig options, or only enabling on new hw, or "merge
and fix the regressions as they come" or any of the other well proven
paths forward.


Yeah, the problem is I don't see an option which currently works for 
everyone.


I absolutely need a grace time of multiple years until we can apply this 
to amdgpu/radeon.


And that under the prerequisite to have a plan to somehow enable that 
functionality now to make it at least painful for userspace to rely on 
hack around that.



So if you want to do this, please start with the mesa side work (as
the biggest userspace, not all of it) with patches to redirect all
primary node rendering to render nodes. The code is there already for
socs, just needs to be rolled out more. Or we go with the DRM_AUTH
horrors. Or a 3rd option, I really don't care which it is, as long as
its consistent.


How about this:
1. We keep DRM_AUTH around for amdgpu/radeon for now.
2. We add a Kconfig option to ignore DRM_AUTH, currently default to N. 
This also works as a workaround for old RADV.
3. We start to work on further removing old cruft from the primary node. 
Possible the Kconfig option I suggested to disable GEM flink.


Regards,
Christian.



tldr; consistent color choice on this bikeshed please.

Thanks, Daniel


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Koenig, Christian]

Am 21.06.19 um 12:53 schrieb Emil Velikov:
> On 2019/06/21, Koenig, Christian wrote:
>> [SNIP]
>> Well partially. That RADV broke is unfortunate, but we have done so many
>> customized userspace stuff both based on Mesa/DDX as well as closed
>> source components that it is just highly likely that we would break
>> something else as well.
>>
> As an engineer I like to work with tangibles. The highly likely part is
> probable, but as mentioned multiple times the series allows for a _dead_
> trivial way to address any such problems.

Well to clarify my concern is that this won't be so trivial.

You implemented on workaround for one specific case and it is perfectly 
possible that for other cases we would have to completely revert the 
removal of DRM_AUTH.

>>> In particular, that user-space will "remove" render nodes.
>> Yes, that is my main concern here. You basically make render nodes
>> superfluously. That gives userspace all legitimization to also remove
>> support for them. That is not stupid or evil or whatever, userspace
>> would just follow the kernel design.
>>
> Do you have an example of userspace doing such an extremely silly thing?
> It does seem like suspect you're a couple of steps beyond overcautious,
> perhaps rightfully so. Maybe you've seen some closed-source user-space
> going crazy? Or any other projects?

The key point is that I don't think this is silly or strange or crazy at 
all. See the kernel defines the interface userspace can and should use.

When the kernel defines that everything will work with the primary node 
it is perfectly valid for userspace to drop support for the render node.

I mean why should they keep this? Just because we tell them not to do this?

> In other words, being cautious is great, but without references of
> misuse it's very hard for others to draw the full picture.
>
>>> I'm really sad that amdgpu insists on standing out, hope one day it will
>>> converge. Yet since all other maintainers are ok with the series, I'll
>>> start merging patches in a few hours. I'm really sad that amdgpu wants
>>> to stand out, hope it will converge sooner rather than later.
>>>
>>> Christian, how would you like amdgpu handled - with a separate flag in
>>> the driver or shall we special case amdgpu within core drm?
>> No, I ask you to please stick around DRM_AUTH for at least amdgpu and
>> radeon. And NOT enable any render node functionality for them on the
>> primary node.
>>
> My question is how do you want this handled:
>   - DRM_PLEASE_FORCE_AUTH - added to AMDGPU/RADEON, or
>   - driver_name == amdgpu, in core DRM.

I want to keep DRM_AUTH in amdgpu and radeon for at least the next 5-10 
years.

At least until we have established that nobody is using the primary node 
for command submission any more. Plus some grace time to make sure that 
this has been spread enough.

Regards,
Christian.

>
>
> Thanks
> Emil

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Daniel Vetter]

On Fri, Jun 21, 2019 at 12:31 PM Koenig, Christian
 wrote:
> Am 21.06.19 um 12:20 schrieb Emil Velikov:
> > In particular, that user-space will "remove" render nodes.
>
> Yes, that is my main concern here. You basically make render nodes
> superfluously. That gives userspace all legitimization to also remove
> support for them. That is not stupid or evil or whatever, userspace
> would just follow the kernel design.

This already happened. At least for kms-only display socs we had to
hide the separate render node (and there you really have to deal with
the separate render node, because it's a distinct driver) entirely
within gbm/mesa.

So if you want to depracate render functionality on primary nodes, you
_have_ to do that hiding in userspace. Or you'll break a lot of
compositors everywhere. Just testing -amdgpu doesn't really prove
anything here. So you won't move the larger ecosystem with this at
all, that ship sailed.

At that point this all feels like a bikeshed, and for a bikeshed I
don't care what the color is we pick, as long as they're all painted
the same.

Once we picked a color it's a simple technical matter of how to roll
it out, using Kconfig options, or only enabling on new hw, or "merge
and fix the regressions as they come" or any of the other well proven
paths forward.

So if you want to do this, please start with the mesa side work (as
the biggest userspace, not all of it) with patches to redirect all
primary node rendering to render nodes. The code is there already for
socs, just needs to be rolled out more. Or we go with the DRM_AUTH
horrors. Or a 3rd option, I really don't care which it is, as long as
its consistent.

tldr; consistent color choice on this bikeshed please.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/21, Koenig, Christian wrote:
> No this should apply to all drivers, not just amdgpu.
> 
> > While I suggested:
> >   - keeping amdgpu consistent with the rest
> >   - exposing the KConfig option for the whole of the kernel, and
> >   - merging it alongside this work
> >
> > So I effectively waited for weeks, instead of simply going ahead and
> > writing/submitting patches. That's a bit unfortunate...
> >
> >>> Because we simply made sure that userspace is using the render node for
> >>> command submission and not the primary node.
> >>>
> >>> So nobody can go ahead and remove render node support any more :)
> >> How does this work? I thought the entire problem of DRM_AUTH removal
> >> for you was that it broke stuff, and you didn't want to deal with
> >> that. We still have that exact problem afaics ... old userspace
> >> doesn't simply vanish, even if you entirely nuke it going forward.
> >>
> >> Also, testing on the amdgpu stack isn't really testing a hole lot
> >> here, it's all the various silly compositors out there I'd expect to
> >> fall over. Will gbm/radeonsi/whatever just internally do all the
> >> necessary dma-buf import/export between the primary node and display
> >> node to keep this all working?
> > If I understood Christian, feel free to correct me, the fact that my
> > earlier patch broke RADV was not the argument. The problem was was the
> > patch does.
> 
> Well partially. That RADV broke is unfortunate, but we have done so many 
> customized userspace stuff both based on Mesa/DDX as well as closed 
> source components that it is just highly likely that we would break 
> something else as well.
> 
As an engineer I like to work with tangibles. The highly likely part is
probable, but as mentioned multiple times the series allows for a _dead_
trivial way to address any such problems.

> > In particular, that user-space will "remove" render nodes.
> 
> Yes, that is my main concern here. You basically make render nodes 
> superfluously. That gives userspace all legitimization to also remove 
> support for them. That is not stupid or evil or whatever, userspace 
> would just follow the kernel design.
> 
Do you have an example of userspace doing such an extremely silly thing?
It does seem like suspect you're a couple of steps beyond overcautious,
perhaps rightfully so. Maybe you've seen some closed-source user-space
going crazy? Or any other projects?

In other words, being cautious is great, but without references of
misuse it's very hard for others to draw the full picture.

> > I'm really sad that amdgpu insists on standing out, hope one day it will
> > converge. Yet since all other maintainers are ok with the series, I'll
> > start merging patches in a few hours. I'm really sad that amdgpu wants
> > to stand out, hope it will converge sooner rather than later.
> >
> > Christian, how would you like amdgpu handled - with a separate flag in
> > the driver or shall we special case amdgpu within core drm?
> 
> No, I ask you to please stick around DRM_AUTH for at least amdgpu and 
> radeon. And NOT enable any render node functionality for them on the 
> primary node.
> 
My question is how do you want this handled:
 - DRM_PLEASE_FORCE_AUTH - added to AMDGPU/RADEON, or
 - driver_name == amdgpu, in core DRM.


Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 09/59] drm/prime: Align gem_prime_export with obj_funcs.export

[Daniel Vetter]

On Fri, Jun 14, 2019 at 10:35:25PM +0200, Daniel Vetter wrote:
> The idea is that gem_prime_export is deprecated in favor of
> obj_funcs.export. That's much easier to do if both have matching
> function signatures.
> 
> Signed-off-by: Daniel Vetter 
> Cc: Russell King 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Sean Paul 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: Zhenyu Wang 
> Cc: Zhi Wang 
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Rodrigo Vivi 
> Cc: Tomi Valkeinen 
> Cc: Alex Deucher 
> Cc: "Christian König" 
> Cc: "David (ChunMing) Zhou" 
> Cc: Thierry Reding 
> Cc: Jonathan Hunter 
> Cc: Dave Airlie 
> Cc: Eric Anholt 
> Cc: "Michel Dänzer" 
> Cc: Chris Wilson 
> Cc: Huang Rui 
> Cc: Felix Kuehling 
> Cc: Hawking Zhang 
> Cc: Feifei Xu 
> Cc: Jim Qu 
> Cc: Evan Quan 
> Cc: Matthew Auld 
> Cc: Mika Kuoppala 
> Cc: Thomas Zimmermann 
> Cc: Kate Stewart 
> Cc: Sumit Semwal 
> Cc: Jilayne Lovejoy 
> Cc: Thomas Gleixner 
> Cc: Mikulas Patocka 
> Cc: Greg Kroah-Hartman 
> Cc: Junwei Zhang 
> Cc: intel-gvt-...@lists.freedesktop.org
> Cc: intel-...@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> Cc: linux-te...@vger.kernel.org

Merged up to this one to drm-misc-next (for 5.4), thanks everyone for the
review and comments thus far.
-Daniel

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c  | 7 +++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h  | 3 +--
>  drivers/gpu/drm/armada/armada_gem.c  | 5 ++---
>  drivers/gpu/drm/armada/armada_gem.h  | 3 +--
>  drivers/gpu/drm/drm_prime.c  | 9 -
>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c   | 5 ++---
>  drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 8 
>  drivers/gpu/drm/i915/gvt/dmabuf.c| 2 +-
>  drivers/gpu/drm/i915/i915_drv.h  | 3 +--
>  drivers/gpu/drm/omapdrm/omap_gem.h   | 3 +--
>  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c| 5 ++---
>  drivers/gpu/drm/radeon/radeon_drv.c  | 3 +--
>  drivers/gpu/drm/radeon/radeon_prime.c| 5 ++---
>  drivers/gpu/drm/tegra/gem.c  | 7 +++
>  drivers/gpu/drm/tegra/gem.h  | 3 +--
>  drivers/gpu/drm/udl/udl_dmabuf.c | 5 ++---
>  drivers/gpu/drm/udl/udl_drv.h| 3 +--
>  drivers/gpu/drm/vc4/vc4_bo.c | 5 ++---
>  drivers/gpu/drm/vc4/vc4_drv.h| 3 +--
>  drivers/gpu/drm/vgem/vgem_fence.c| 2 +-
>  include/drm/drm_drv.h| 4 ++--
>  include/drm/drm_prime.h  | 3 +--
>  22 files changed, 39 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index 489041df1f45..4809d4a5d72a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -345,8 +345,7 @@ const struct dma_buf_ops amdgpu_dmabuf_ops = {
>   * Returns:
>   * Shared DMA buffer representing the GEM BO from the given device.
>   */
> -struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
> - struct drm_gem_object *gobj,
> +struct dma_buf *amdgpu_gem_prime_export(struct drm_gem_object *gobj,
>   int flags)
>  {
>   struct amdgpu_bo *bo = gem_to_amdgpu_bo(gobj);
> @@ -356,9 +355,9 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_device 
> *dev,
>   bo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)
>   return ERR_PTR(-EPERM);
>  
> - buf = drm_gem_prime_export(dev, gobj, flags);
> + buf = drm_gem_prime_export(gobj, flags);
>   if (!IS_ERR(buf)) {
> - buf->file->f_mapping = dev->anon_inode->i_mapping;
> + buf->file->f_mapping = gobj->dev->anon_inode->i_mapping;
>   buf->ops = &amdgpu_dmabuf_ops;
>   }
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
> index c7056cbe8685..7f73a4f94204 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
> @@ -30,8 +30,7 @@ struct drm_gem_object *
>  amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>struct dma_buf_attachment *attach,
>struct sg_table *sg);
> -struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
> - struct drm_gem_object *gobj,
> +struct dma_buf *amdgpu_gem_prime_export(struct drm_gem_object *gobj,
>   int flags);
>  struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
>   struct dma_buf *dma_buf);
> diff --git a/drivers/gpu/drm/armada/armada_gem.c 
> b/drivers/g

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Koenig, Christian]

Am 21.06.19 um 12:20 schrieb Emil Velikov:
> On 2019/06/21, Daniel Vetter wrote:
>> On Fri, Jun 21, 2019 at 11:25 AM Koenig, Christian
>>  wrote:
>>> Am 21.06.19 um 11:09 schrieb Daniel Vetter:
 On Fri, Jun 21, 2019 at 07:12:14AM +, Koenig, Christian wrote:
> Am 20.06.19 um 18:30 schrieb Emil Velikov:
>> On 2019/06/14, Koenig, Christian wrote:
>>> Am 14.06.19 um 17:53 schrieb Emil Velikov:
 On 2019/06/14, Koenig, Christian wrote:
> Am 14.06.19 um 14:09 schrieb Emil Velikov:
>> On 2019/05/27, Emil Velikov wrote:
>> [SNIP]
>> Hi Christian,
>>
>>
>> In the following, I would like to summarise and emphasize the need 
>> for
>> DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
>> extra reading it.
>>
>>
>> Today DRM drivers* do not make any distinction between primary and
>> render node clients.
> That is actually not 100% correct. We have a special case where a DRM
> master is allowed to change the priority of render node clients.
>
 Can you provide a link? I cannot find that code.
>>> See amdgpu_sched_ioctl().
>>>
>> Thus for a render capable driver, any premise of
>> separation, security or otherwise imposed via DRM_AUTH is a fallacy.
> Yeah, that's what I agree on. I just don't think that removing 
> DRM_AUTH
> now is the right direction to take.
>
 Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
 ioctls.

 That aside, can you propose an alternative solution that addresses this
 and the second point just below?
>>> Give me a few days to work on this, it's already Friday 6pm here.
>>>
>> Hi Christian,
>>
>> Any progress? As mentioned earlier, I'm OK with writing the patches 
>> although
>> I would love to hear your plan.
> First of all I tried to disable DRM authentication completely with a
> kernel config option. Surprisingly that actually works out of the box at
> least on the AMDGPU stack.
>
> This effectively allows us to get rid of DRI2 and the related security
> problems. Only thing left for that is that I'm just not sure how to
> signal this to userspace so that the DDX wouldn't advertise DRI2 at all
> any more.
>
>
> As a next step I looked into if we can disable the command submission
> for DRM master. Turned out that this is relatively easy as well.
>
> All we have to do is to fix the bug Michel pointed out about KMS handles
> for display and let the DDX use a render node instead of the DRM master
> for Glamor. Still need to sync up with Michel and/or Marek whats the
> best way of doing this.
>
>
> The last step (and that's what I haven't tried yet) would be to disable
> DRM authentication for Navi even when it is still compiled into the
> kernel. But that is more or less just a typing exercise.
 So just to get this straight, we're now full on embracing a subsystem
 split here:
 - amdgpu plans to ditch dri2/rendering on primary nodes
 - bunch of drivers (I think more than i915 by now) merged the DRM_AUTH
 removal
 - others are just hanging in there somehow

 "best of both^W worlds" ftw?
>>> Yes, exactly!
>>>
>>> Think a step further, when this is upstream we can apply the DRM_AUTH
>>> removal to amdgpu as well.
>>>
> So this is effectively what I suggested/planned with a couple of caveats:
>   - making amdgpu behave differently from the rest of drm ;-(
>   - having the KConfig amdgpu only and merged before this DRM_AUTH

No this should apply to all drivers, not just amdgpu.

> While I suggested:
>   - keeping amdgpu consistent with the rest
>   - exposing the KConfig option for the whole of the kernel, and
>   - merging it alongside this work
>
> So I effectively waited for weeks, instead of simply going ahead and
> writing/submitting patches. That's a bit unfortunate...
>
>>> Because we simply made sure that userspace is using the render node for
>>> command submission and not the primary node.
>>>
>>> So nobody can go ahead and remove render node support any more :)
>> How does this work? I thought the entire problem of DRM_AUTH removal
>> for you was that it broke stuff, and you didn't want to deal with
>> that. We still have that exact problem afaics ... old userspace
>> doesn't simply vanish, even if you entirely nuke it going forward.
>>
>> Also, testing on the amdgpu stack isn't really testing a hole lot
>> here, it's all the various silly compositors out there I'd expect to
>> fall over. Will gbm/radeonsi/whatever just internally do all the
>> necessary dma-buf import/export between the primary node and display
>> node to keep this all working?
> If I understood Christian, feel free to correct me, the fact that my
> earlier patch broke

Re: [Intel-gfx] [PATCH 1/6] dma-buf: add dynamic DMA-buf handling v10

[Daniel Vetter]

On Fri, Jun 21, 2019 at 11:55 AM Christian König
 wrote:
>
> Am 21.06.19 um 11:20 schrieb Daniel Vetter:
> > On Tue, Jun 18, 2019 at 01:54:50PM +0200, Christian König wrote:
> >> On the exporter side we add optional explicit pinning callbacks. If those
> >> callbacks are implemented the framework no longer caches sg tables and the
> >> map/unmap callbacks are always called with the lock of the reservation 
> >> object
> >> held.
> >>
> >> On the importer side we add an optional invalidate callback. This callback 
> >> is
> >> used by the exporter to inform the importers that their mappings should be
> >> destroyed as soon as possible.
> >>
> >> This allows the exporter to provide the mappings without the need to pin
> >> the backing store.
> >>
> >> v2: don't try to invalidate mappings when the callback is NULL,
> >>  lock the reservation obj while using the attachments,
> >>  add helper to set the callback
> >> v3: move flag for invalidation support into the DMA-buf,
> >>  use new attach_info structure to set the callback
> >> v4: use importer_priv field instead of mangling exporter priv.
> >> v5: drop invalidation_supported flag
> >> v6: squash together with pin/unpin changes
> >> v7: pin/unpin takes an attachment now
> >> v8: nuke dma_buf_attachment_(map|unmap)_locked,
> >>  everything is now handled backward compatible
> >> v9: always cache when export/importer don't agree on dynamic handling
> >> v10: minimal style cleanup
> >>
> >> Signed-off-by: Christian König 
> >> ---
> >>   drivers/dma-buf/dma-buf.c | 188 --
> >>   include/linux/dma-buf.h   | 109 --
> >>   2 files changed, 283 insertions(+), 14 deletions(-)
> >>
> >> [SNIP]
> >> +if (dma_buf_attachment_is_dynamic(attach)) {
> >> +reservation_object_assert_held(attach->dmabuf->resv);
> >> +
> >> +/*
> >> + * Mapping a DMA-buf can trigger its invalidation, prevent
> >> + * sending this event to the caller by temporary removing
> >> + * this attachment from the list.
> >> + */
> >> +list_del(&attach->node);
> > I'm still hung up about this, that still feels like leaking random ttm
> > implementation details into the dma-buf interfaces. And it's asymmetric:
> >
> > - When acquiring a buffer mapping (whether p2p or system memory sg or
> >whatever) we always have to wait for pending fences before we can access
> >the buffer. At least for full dynamic dma-buf access.
> >
> > - Same is true when dropping a mapping: We could drop the mapping
> >immediately, but only actually release it when that fence has signalled.
> >Then this hack here wouldn't be necessary.
> >
> > It feels a bit like this is just an artifact of how ttm currently does bo
> > moves with the shadow bo. There's other ways to fix that, you could just
> > have a memory manager reservation of a given range or whatever and a
> > release fence from when it's actually good to use.
>
> No, that is for handling a completely different case :)
>
> >
> > Imo the below semantics would be much cleaner:
> >
> > - invalidate may add new fences
> > - invalidate _must_ unmap its mappings
> > - an unmap must wait for current fences before the mapping can be
> >released.
> >
> > Imo there's no reason why unmap is special, and the only thing where we
> > don't use fences to gate access to resources/memory when it's in the
> > process of getting moved around.
>
> Well in general I want to avoid waiting for fences as much as possible.
> But the key point here is that this actually won't help with the problem
> I'm trying to solve.

The point of using fences is not to wait on them. I mean if you have
the shadow ttm bo on the lru you also don't wait for that fence to
retire before you insert the shadow bo onto the lru. You don't even
wait when you try to use that memory again, you just pipeline more
stuff on top.

In the end it will be the exact same amount of fences and waiting in
both solutions. One just leaks less implementationt details (at least
in my opinion) across the dma-buf border.

> > btw this is like the 2nd or 3rd time I'm typing this, haven't seen your
> > thoughts on this yet.
>
> Yeah, and I'm responding for the 3rd time now that you are
> misunderstanding why we need this here :)
>
> Maybe I can make that clear with an example:
>
> 1. You got a sharing between device A (exporter) and B (importer) which
> uses P2P.
>
> 2. Now device C (importer) comes along and wants to use the DMA-buf
> object as well.
>
> 3. The handling now figures out that we can't do P2P between device A
> and device C (for whatever reason).
>
> 4. The map_attachment implementation in device driver A doesn't want to
> fail with -EBUSY and migrates the DMA-buf somewhere where both device A
> and device C can access it.
>
> 5. This migration will result in sending an invalidation event around.
> And here it doesn't make sense to send this invalida

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/21, Daniel Vetter wrote:
> On Fri, Jun 21, 2019 at 11:25 AM Koenig, Christian
>  wrote:
> >
> > Am 21.06.19 um 11:09 schrieb Daniel Vetter:
> > > On Fri, Jun 21, 2019 at 07:12:14AM +, Koenig, Christian wrote:
> > >> Am 20.06.19 um 18:30 schrieb Emil Velikov:
> > >>> On 2019/06/14, Koenig, Christian wrote:
> >  Am 14.06.19 um 17:53 schrieb Emil Velikov:
> > > On 2019/06/14, Koenig, Christian wrote:
> > >> Am 14.06.19 um 14:09 schrieb Emil Velikov:
> > >>> On 2019/05/27, Emil Velikov wrote:
> > >>> [SNIP]
> > >>> Hi Christian,
> > >>>
> > >>>
> > >>> In the following, I would like to summarise and emphasize the need 
> > >>> for
> > >>> DRM_AUTH removal. I would kindly ask you to spend a couple of 
> > >>> minutes
> > >>> extra reading it.
> > >>>
> > >>>
> > >>> Today DRM drivers* do not make any distinction between primary and
> > >>> render node clients.
> > >> That is actually not 100% correct. We have a special case where a DRM
> > >> master is allowed to change the priority of render node clients.
> > >>
> > > Can you provide a link? I cannot find that code.
> >  See amdgpu_sched_ioctl().
> > 
> > >>> Thus for a render capable driver, any premise of
> > >>> separation, security or otherwise imposed via DRM_AUTH is a fallacy.
> > >> Yeah, that's what I agree on. I just don't think that removing 
> > >> DRM_AUTH
> > >> now is the right direction to take.
> > >>
> > > Could have been clearer - I'm talking about DRM_AUTH | 
> > > DRM_RENDER_ALLOW
> > > ioctls.
> > >
> > > That aside, can you propose an alternative solution that addresses 
> > > this
> > > and the second point just below?
> >  Give me a few days to work on this, it's already Friday 6pm here.
> > 
> > >>> Hi Christian,
> > >>>
> > >>> Any progress? As mentioned earlier, I'm OK with writing the patches 
> > >>> although
> > >>> I would love to hear your plan.
> > >> First of all I tried to disable DRM authentication completely with a
> > >> kernel config option. Surprisingly that actually works out of the box at
> > >> least on the AMDGPU stack.
> > >>
> > >> This effectively allows us to get rid of DRI2 and the related security
> > >> problems. Only thing left for that is that I'm just not sure how to
> > >> signal this to userspace so that the DDX wouldn't advertise DRI2 at all
> > >> any more.
> > >>
> > >>
> > >> As a next step I looked into if we can disable the command submission
> > >> for DRM master. Turned out that this is relatively easy as well.
> > >>
> > >> All we have to do is to fix the bug Michel pointed out about KMS handles
> > >> for display and let the DDX use a render node instead of the DRM master
> > >> for Glamor. Still need to sync up with Michel and/or Marek whats the
> > >> best way of doing this.
> > >>
> > >>
> > >> The last step (and that's what I haven't tried yet) would be to disable
> > >> DRM authentication for Navi even when it is still compiled into the
> > >> kernel. But that is more or less just a typing exercise.
> > > So just to get this straight, we're now full on embracing a subsystem
> > > split here:
> > > - amdgpu plans to ditch dri2/rendering on primary nodes
> > > - bunch of drivers (I think more than i915 by now) merged the DRM_AUTH
> > >removal
> > > - others are just hanging in there somehow
> > >
> > > "best of both^W worlds" ftw?
> >
> > Yes, exactly!
> >
> > Think a step further, when this is upstream we can apply the DRM_AUTH
> > removal to amdgpu as well.
> >
So this is effectively what I suggested/planned with a couple of caveats:
 - making amdgpu behave differently from the rest of drm ;-(
 - having the KConfig amdgpu only and merged before this DRM_AUTH

While I suggested:
 - keeping amdgpu consistent with the rest
 - exposing the KConfig option for the whole of the kernel, and
 - merging it alongside this work

So I effectively waited for weeks, instead of simply going ahead and
writing/submitting patches. That's a bit unfortunate...

> > Because we simply made sure that userspace is using the render node for
> > command submission and not the primary node.
> >
> > So nobody can go ahead and remove render node support any more :)
> 
> How does this work? I thought the entire problem of DRM_AUTH removal
> for you was that it broke stuff, and you didn't want to deal with
> that. We still have that exact problem afaics ... old userspace
> doesn't simply vanish, even if you entirely nuke it going forward.
> 
> Also, testing on the amdgpu stack isn't really testing a hole lot
> here, it's all the various silly compositors out there I'd expect to
> fall over. Will gbm/radeonsi/whatever just internally do all the
> necessary dma-buf import/export between the primary node and display
> node to keep this all working?

If I understood Christian, feel free to correct me, the fact that my
earlier patch broke RADV was not the arg

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Christian König]

Am 21.06.19 um 11:35 schrieb Daniel Vetter:

On Fri, Jun 21, 2019 at 11:25 AM Koenig, Christian
 wrote:

Am 21.06.19 um 11:09 schrieb Daniel Vetter:

On Fri, Jun 21, 2019 at 07:12:14AM +, Koenig, Christian wrote:

Am 20.06.19 um 18:30 schrieb Emil Velikov:

On 2019/06/14, Koenig, Christian wrote:

Am 14.06.19 um 17:53 schrieb Emil Velikov:

On 2019/06/14, Koenig, Christian wrote:

Am 14.06.19 um 14:09 schrieb Emil Velikov:

On 2019/05/27, Emil Velikov wrote:
[SNIP]
Hi Christian,


In the following, I would like to summarise and emphasize the need for
DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
extra reading it.


Today DRM drivers* do not make any distinction between primary and
render node clients.

That is actually not 100% correct. We have a special case where a DRM
master is allowed to change the priority of render node clients.


Can you provide a link? I cannot find that code.

See amdgpu_sched_ioctl().


Thus for a render capable driver, any premise of
separation, security or otherwise imposed via DRM_AUTH is a fallacy.

Yeah, that's what I agree on. I just don't think that removing DRM_AUTH
now is the right direction to take.


Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
ioctls.

That aside, can you propose an alternative solution that addresses this
and the second point just below?

Give me a few days to work on this, it's already Friday 6pm here.


Hi Christian,

Any progress? As mentioned earlier, I'm OK with writing the patches although
I would love to hear your plan.

First of all I tried to disable DRM authentication completely with a
kernel config option. Surprisingly that actually works out of the box at
least on the AMDGPU stack.

This effectively allows us to get rid of DRI2 and the related security
problems. Only thing left for that is that I'm just not sure how to
signal this to userspace so that the DDX wouldn't advertise DRI2 at all
any more.


As a next step I looked into if we can disable the command submission
for DRM master. Turned out that this is relatively easy as well.

All we have to do is to fix the bug Michel pointed out about KMS handles
for display and let the DDX use a render node instead of the DRM master
for Glamor. Still need to sync up with Michel and/or Marek whats the
best way of doing this.


The last step (and that's what I haven't tried yet) would be to disable
DRM authentication for Navi even when it is still compiled into the
kernel. But that is more or less just a typing exercise.

So just to get this straight, we're now full on embracing a subsystem
split here:
- amdgpu plans to ditch dri2/rendering on primary nodes
- bunch of drivers (I think more than i915 by now) merged the DRM_AUTH
removal
- others are just hanging in there somehow

"best of both^W worlds" ftw?

Yes, exactly!

Think a step further, when this is upstream we can apply the DRM_AUTH
removal to amdgpu as well.

Because we simply made sure that userspace is using the render node for
command submission and not the primary node.

So nobody can go ahead and remove render node support any more :)

How does this work? I thought the entire problem of DRM_AUTH removal
for you was that it broke stuff, and you didn't want to deal with
that.


Yeah, that is indeed still true.

It's just that we have done way to many projects with radeon/amdgpu and 
customized userspace stuff.



We still have that exact problem afaics ... old userspace
doesn't simply vanish, even if you entirely nuke it going forward.


Well old userspace doesn't work with new hardware either.

So the idea is to keep old userspace for old hardware working, but only 
disable old stuff for new hardware.



Also, testing on the amdgpu stack isn't really testing a hole lot
here, it's all the various silly compositors out there I'd expect to
fall over. Will gbm/radeonsi/whatever just internally do all the
necessary dma-buf import/export between the primary node and display
node to keep this all working?


Yes, at least that's how I understand Michel's idea.

We keep both file descriptors for primary and render node around at the 
same time anyway. So the change is actually not that much.


This also solves the problem that people are running things as root, 
since we now always use the render node and never the primary node for 
everything except KMS.


Christian.


-Daniel


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-gfx] [PATCH 1/6] dma-buf: add dynamic DMA-buf handling v10

[Christian König]

Am 21.06.19 um 11:20 schrieb Daniel Vetter:

On Tue, Jun 18, 2019 at 01:54:50PM +0200, Christian König wrote:

On the exporter side we add optional explicit pinning callbacks. If those
callbacks are implemented the framework no longer caches sg tables and the
map/unmap callbacks are always called with the lock of the reservation object
held.

On the importer side we add an optional invalidate callback. This callback is
used by the exporter to inform the importers that their mappings should be
destroyed as soon as possible.

This allows the exporter to provide the mappings without the need to pin
the backing store.

v2: don't try to invalidate mappings when the callback is NULL,
 lock the reservation obj while using the attachments,
 add helper to set the callback
v3: move flag for invalidation support into the DMA-buf,
 use new attach_info structure to set the callback
v4: use importer_priv field instead of mangling exporter priv.
v5: drop invalidation_supported flag
v6: squash together with pin/unpin changes
v7: pin/unpin takes an attachment now
v8: nuke dma_buf_attachment_(map|unmap)_locked,
 everything is now handled backward compatible
v9: always cache when export/importer don't agree on dynamic handling
v10: minimal style cleanup

Signed-off-by: Christian König 
---
  drivers/dma-buf/dma-buf.c | 188 --
  include/linux/dma-buf.h   | 109 --
  2 files changed, 283 insertions(+), 14 deletions(-)

[SNIP]
+   if (dma_buf_attachment_is_dynamic(attach)) {
+   reservation_object_assert_held(attach->dmabuf->resv);
+
+   /*
+* Mapping a DMA-buf can trigger its invalidation, prevent
+* sending this event to the caller by temporary removing
+* this attachment from the list.
+*/
+   list_del(&attach->node);

I'm still hung up about this, that still feels like leaking random ttm
implementation details into the dma-buf interfaces. And it's asymmetric:

- When acquiring a buffer mapping (whether p2p or system memory sg or
   whatever) we always have to wait for pending fences before we can access
   the buffer. At least for full dynamic dma-buf access.

- Same is true when dropping a mapping: We could drop the mapping
   immediately, but only actually release it when that fence has signalled.
   Then this hack here wouldn't be necessary.

It feels a bit like this is just an artifact of how ttm currently does bo
moves with the shadow bo. There's other ways to fix that, you could just
have a memory manager reservation of a given range or whatever and a
release fence from when it's actually good to use.


No, that is for handling a completely different case :)



Imo the below semantics would be much cleaner:

- invalidate may add new fences
- invalidate _must_ unmap its mappings
- an unmap must wait for current fences before the mapping can be
   released.

Imo there's no reason why unmap is special, and the only thing where we
don't use fences to gate access to resources/memory when it's in the
process of getting moved around.


Well in general I want to avoid waiting for fences as much as possible. 
But the key point here is that this actually won't help with the problem 
I'm trying to solve.



btw this is like the 2nd or 3rd time I'm typing this, haven't seen your
thoughts on this yet.


Yeah, and I'm responding for the 3rd time now that you are 
misunderstanding why we need this here :)


Maybe I can make that clear with an example:

1. You got a sharing between device A (exporter) and B (importer) which 
uses P2P.


2. Now device C (importer) comes along and wants to use the DMA-buf 
object as well.


3. The handling now figures out that we can't do P2P between device A 
and device C (for whatever reason).


4. The map_attachment implementation in device driver A doesn't want to 
fail with -EBUSY and migrates the DMA-buf somewhere where both device A 
and device C can access it.


5. This migration will result in sending an invalidation event around. 
And here it doesn't make sense to send this invalidation event to device 
C, because we know that device C is actually causing this event and 
doesn't have a valid mapping.



One alternative would be to completely disallow buffer migration which 
can cause invalidation in the drivers map_attachment call. But with 
dynamic handling you definitely need to be able to migrate in the 
map_attachment call for swapping evicted things back into a place where 
they are accessible. So that would make it harder for drivers to get it 
right.



Another alternative (and that's what I implemented initially) is to make 
sure the driver calling map_attachment can handle invalidation events 
re-entering itself while doing so. But then you add another tricky thing 
for drivers to handle which could be done in the general code.



The reason I don't have that on unmap is that I think migrating things 
on unmap 

[PATCH] drm/amdgpu: add sw_init to df_v1_7

[Kim, Jonathan]

add df sw init to df 1.7 function to prevent regression issues on pre-vega20
products.

Change-Id: I4941003ea4a99ba0ea736c7ecc8800148423c379
Signed-off-by: Jonathan Kim 
---
 drivers/gpu/drm/amd/amdgpu/df_v1_7.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/df_v1_7.c 
b/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
index 9935371db7ce..335f2c02878f 100644
--- a/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
+++ b/drivers/gpu/drm/amd/amdgpu/df_v1_7.c
@@ -33,6 +33,10 @@ static void df_v1_7_init (struct amdgpu_device *adev)
 {
 }
 
+static void df_v1_7_sw_init(struct amdgpu_device *adev)
+{
+}
+
 static void df_v1_7_enable_broadcast_mode(struct amdgpu_device *adev,
   bool enable)
 {
@@ -111,6 +115,7 @@ static void df_v1_7_enable_ecc_force_par_wr_rmw(struct 
amdgpu_device *adev,
 
 const struct amdgpu_df_funcs df_v1_7_funcs = {
.init = df_v1_7_init,
+   .sw_init = df_v1_7_sw_init,
.enable_broadcast_mode = df_v1_7_enable_broadcast_mode,
.get_fb_channel_number = df_v1_7_get_fb_channel_number,
.get_hbm_channel_number = df_v1_7_get_hbm_channel_number,
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Daniel Vetter]

On Fri, Jun 21, 2019 at 11:25 AM Koenig, Christian
 wrote:
>
> Am 21.06.19 um 11:09 schrieb Daniel Vetter:
> > On Fri, Jun 21, 2019 at 07:12:14AM +, Koenig, Christian wrote:
> >> Am 20.06.19 um 18:30 schrieb Emil Velikov:
> >>> On 2019/06/14, Koenig, Christian wrote:
>  Am 14.06.19 um 17:53 schrieb Emil Velikov:
> > On 2019/06/14, Koenig, Christian wrote:
> >> Am 14.06.19 um 14:09 schrieb Emil Velikov:
> >>> On 2019/05/27, Emil Velikov wrote:
> >>> [SNIP]
> >>> Hi Christian,
> >>>
> >>>
> >>> In the following, I would like to summarise and emphasize the need for
> >>> DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
> >>> extra reading it.
> >>>
> >>>
> >>> Today DRM drivers* do not make any distinction between primary and
> >>> render node clients.
> >> That is actually not 100% correct. We have a special case where a DRM
> >> master is allowed to change the priority of render node clients.
> >>
> > Can you provide a link? I cannot find that code.
>  See amdgpu_sched_ioctl().
> 
> >>> Thus for a render capable driver, any premise of
> >>> separation, security or otherwise imposed via DRM_AUTH is a fallacy.
> >> Yeah, that's what I agree on. I just don't think that removing DRM_AUTH
> >> now is the right direction to take.
> >>
> > Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
> > ioctls.
> >
> > That aside, can you propose an alternative solution that addresses this
> > and the second point just below?
>  Give me a few days to work on this, it's already Friday 6pm here.
> 
> >>> Hi Christian,
> >>>
> >>> Any progress? As mentioned earlier, I'm OK with writing the patches 
> >>> although
> >>> I would love to hear your plan.
> >> First of all I tried to disable DRM authentication completely with a
> >> kernel config option. Surprisingly that actually works out of the box at
> >> least on the AMDGPU stack.
> >>
> >> This effectively allows us to get rid of DRI2 and the related security
> >> problems. Only thing left for that is that I'm just not sure how to
> >> signal this to userspace so that the DDX wouldn't advertise DRI2 at all
> >> any more.
> >>
> >>
> >> As a next step I looked into if we can disable the command submission
> >> for DRM master. Turned out that this is relatively easy as well.
> >>
> >> All we have to do is to fix the bug Michel pointed out about KMS handles
> >> for display and let the DDX use a render node instead of the DRM master
> >> for Glamor. Still need to sync up with Michel and/or Marek whats the
> >> best way of doing this.
> >>
> >>
> >> The last step (and that's what I haven't tried yet) would be to disable
> >> DRM authentication for Navi even when it is still compiled into the
> >> kernel. But that is more or less just a typing exercise.
> > So just to get this straight, we're now full on embracing a subsystem
> > split here:
> > - amdgpu plans to ditch dri2/rendering on primary nodes
> > - bunch of drivers (I think more than i915 by now) merged the DRM_AUTH
> >removal
> > - others are just hanging in there somehow
> >
> > "best of both^W worlds" ftw?
>
> Yes, exactly!
>
> Think a step further, when this is upstream we can apply the DRM_AUTH
> removal to amdgpu as well.
>
> Because we simply made sure that userspace is using the render node for
> command submission and not the primary node.
>
> So nobody can go ahead and remove render node support any more :)

How does this work? I thought the entire problem of DRM_AUTH removal
for you was that it broke stuff, and you didn't want to deal with
that. We still have that exact problem afaics ... old userspace
doesn't simply vanish, even if you entirely nuke it going forward.

Also, testing on the amdgpu stack isn't really testing a hole lot
here, it's all the various silly compositors out there I'd expect to
fall over. Will gbm/radeonsi/whatever just internally do all the
necessary dma-buf import/export between the primary node and display
node to keep this all working?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Koenig, Christian]

Am 21.06.19 um 11:09 schrieb Daniel Vetter:
> On Fri, Jun 21, 2019 at 07:12:14AM +, Koenig, Christian wrote:
>> Am 20.06.19 um 18:30 schrieb Emil Velikov:
>>> On 2019/06/14, Koenig, Christian wrote:
 Am 14.06.19 um 17:53 schrieb Emil Velikov:
> On 2019/06/14, Koenig, Christian wrote:
>> Am 14.06.19 um 14:09 schrieb Emil Velikov:
>>> On 2019/05/27, Emil Velikov wrote:
>>> [SNIP]
>>> Hi Christian,
>>>
>>>
>>> In the following, I would like to summarise and emphasize the need for
>>> DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
>>> extra reading it.
>>>
>>>
>>> Today DRM drivers* do not make any distinction between primary and
>>> render node clients.
>> That is actually not 100% correct. We have a special case where a DRM
>> master is allowed to change the priority of render node clients.
>>
> Can you provide a link? I cannot find that code.
 See amdgpu_sched_ioctl().

>>> Thus for a render capable driver, any premise of
>>> separation, security or otherwise imposed via DRM_AUTH is a fallacy.
>> Yeah, that's what I agree on. I just don't think that removing DRM_AUTH
>> now is the right direction to take.
>>
> Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
> ioctls.
>
> That aside, can you propose an alternative solution that addresses this
> and the second point just below?
 Give me a few days to work on this, it's already Friday 6pm here.

>>> Hi Christian,
>>>
>>> Any progress? As mentioned earlier, I'm OK with writing the patches although
>>> I would love to hear your plan.
>> First of all I tried to disable DRM authentication completely with a
>> kernel config option. Surprisingly that actually works out of the box at
>> least on the AMDGPU stack.
>>
>> This effectively allows us to get rid of DRI2 and the related security
>> problems. Only thing left for that is that I'm just not sure how to
>> signal this to userspace so that the DDX wouldn't advertise DRI2 at all
>> any more.
>>
>>
>> As a next step I looked into if we can disable the command submission
>> for DRM master. Turned out that this is relatively easy as well.
>>
>> All we have to do is to fix the bug Michel pointed out about KMS handles
>> for display and let the DDX use a render node instead of the DRM master
>> for Glamor. Still need to sync up with Michel and/or Marek whats the
>> best way of doing this.
>>
>>
>> The last step (and that's what I haven't tried yet) would be to disable
>> DRM authentication for Navi even when it is still compiled into the
>> kernel. But that is more or less just a typing exercise.
> So just to get this straight, we're now full on embracing a subsystem
> split here:
> - amdgpu plans to ditch dri2/rendering on primary nodes
> - bunch of drivers (I think more than i915 by now) merged the DRM_AUTH
>removal
> - others are just hanging in there somehow
>
> "best of both^W worlds" ftw?

Yes, exactly!

Think a step further, when this is upstream we can apply the DRM_AUTH 
removal to amdgpu as well.

Because we simply made sure that userspace is using the render node for 
command submission and not the primary node.

So nobody can go ahead and remove render node support any more :)

Regards,
Christian.

> -Daniel

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-gfx] [PATCH 1/6] dma-buf: add dynamic DMA-buf handling v10

[Daniel Vetter]

On Tue, Jun 18, 2019 at 01:54:50PM +0200, Christian König wrote:
> On the exporter side we add optional explicit pinning callbacks. If those
> callbacks are implemented the framework no longer caches sg tables and the
> map/unmap callbacks are always called with the lock of the reservation object
> held.
> 
> On the importer side we add an optional invalidate callback. This callback is
> used by the exporter to inform the importers that their mappings should be
> destroyed as soon as possible.
> 
> This allows the exporter to provide the mappings without the need to pin
> the backing store.
> 
> v2: don't try to invalidate mappings when the callback is NULL,
> lock the reservation obj while using the attachments,
> add helper to set the callback
> v3: move flag for invalidation support into the DMA-buf,
> use new attach_info structure to set the callback
> v4: use importer_priv field instead of mangling exporter priv.
> v5: drop invalidation_supported flag
> v6: squash together with pin/unpin changes
> v7: pin/unpin takes an attachment now
> v8: nuke dma_buf_attachment_(map|unmap)_locked,
> everything is now handled backward compatible
> v9: always cache when export/importer don't agree on dynamic handling
> v10: minimal style cleanup
> 
> Signed-off-by: Christian König 
> ---
>  drivers/dma-buf/dma-buf.c | 188 --
>  include/linux/dma-buf.h   | 109 --
>  2 files changed, 283 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 6c15deb5d4ad..9c9c95a88655 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -531,6 +531,9 @@ struct dma_buf *dma_buf_export(const struct 
> dma_buf_export_info *exp_info)
>   return ERR_PTR(-EINVAL);
>   }
>  
> + if (WARN_ON(exp_info->ops->cache_sgt_mapping && exp_info->ops->pin))
> + return ERR_PTR(-EINVAL);
> +
>   if (!try_module_get(exp_info->owner))
>   return ERR_PTR(-ENOENT);
>  
> @@ -651,10 +654,12 @@ void dma_buf_put(struct dma_buf *dmabuf)
>  EXPORT_SYMBOL_GPL(dma_buf_put);
>  
>  /**
> - * dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
> + * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list; 
> optionally,
>   * calls attach() of dma_buf_ops to allow device-specific attach 
> functionality
> - * @dmabuf:  [in]buffer to attach device to.
> - * @dev: [in]device to be attached.
> + * @dmabuf:  [in]buffer to attach device to.
> + * @dev: [in]device to be attached.
> + * @importer_ops [in]importer operations for the attachment
> + * @importer_priv[in]importer private pointer for the attachment
>   *
>   * Returns struct dma_buf_attachment pointer for this attachment. Attachments
>   * must be cleaned up by calling dma_buf_detach().
> @@ -668,8 +673,10 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
>   * accessible to @dev, and cannot be moved to a more suitable place. This is
>   * indicated with the error code -EBUSY.
>   */
> -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> -   struct device *dev)
> +struct dma_buf_attachment *
> +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
> +const struct dma_buf_attach_ops *importer_ops,
> +void *importer_priv)
>  {
>   struct dma_buf_attachment *attach;
>   int ret;
> @@ -683,6 +690,8 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf 
> *dmabuf,
>  
>   attach->dev = dev;
>   attach->dmabuf = dmabuf;
> + attach->importer_ops = importer_ops;
> + attach->importer_priv = importer_priv;
>  
>   mutex_lock(&dmabuf->lock);
>  
> @@ -691,16 +700,72 @@ struct dma_buf_attachment *dma_buf_attach(struct 
> dma_buf *dmabuf,
>   if (ret)
>   goto err_attach;
>   }
> + reservation_object_lock(dmabuf->resv, NULL);
>   list_add(&attach->node, &dmabuf->attachments);
> + reservation_object_unlock(dmabuf->resv);
>  
>   mutex_unlock(&dmabuf->lock);
>  
> + /* When either the importer or the exporter can't handle dynamic
> +  * mappings we cache the mapping here to avoid issues with the
> +  * reservation object lock.
> +  */
> + if (dma_buf_attachment_is_dynamic(attach) !=
> + dma_buf_is_dynamic(dmabuf)) {
> + struct sg_table *sgt;
> +
> + if (dma_buf_is_dynamic(attach->dmabuf)) {
> + reservation_object_lock(attach->dmabuf->resv, NULL);
> + ret = dma_buf_pin(attach);
> + if (ret)
> + goto err_unlock;
> + }
> +
> + sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
> + if (!sgt)
> + sgt = ERR_PTR(-ENOMEM);
> + if (IS_ERR(sgt)) {
> + 

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Daniel Vetter]

On Fri, Jun 21, 2019 at 07:12:14AM +, Koenig, Christian wrote:
> Am 20.06.19 um 18:30 schrieb Emil Velikov:
> > On 2019/06/14, Koenig, Christian wrote:
> >> Am 14.06.19 um 17:53 schrieb Emil Velikov:
> >>> On 2019/06/14, Koenig, Christian wrote:
>  Am 14.06.19 um 14:09 schrieb Emil Velikov:
> > On 2019/05/27, Emil Velikov wrote:
> > [SNIP]
> > Hi Christian,
> >
> >
> > In the following, I would like to summarise and emphasize the need for
> > DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
> > extra reading it.
> >
> >
> > Today DRM drivers* do not make any distinction between primary and
> > render node clients.
>  That is actually not 100% correct. We have a special case where a DRM
>  master is allowed to change the priority of render node clients.
> 
> >>> Can you provide a link? I cannot find that code.
> >> See amdgpu_sched_ioctl().
> >>
> > Thus for a render capable driver, any premise of
> > separation, security or otherwise imposed via DRM_AUTH is a fallacy.
>  Yeah, that's what I agree on. I just don't think that removing DRM_AUTH
>  now is the right direction to take.
> 
> >>> Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
> >>> ioctls.
> >>>
> >>> That aside, can you propose an alternative solution that addresses this
> >>> and the second point just below?
> >> Give me a few days to work on this, it's already Friday 6pm here.
> >>
> > Hi Christian,
> >
> > Any progress? As mentioned earlier, I'm OK with writing the patches although
> > I would love to hear your plan.
> 
> First of all I tried to disable DRM authentication completely with a 
> kernel config option. Surprisingly that actually works out of the box at 
> least on the AMDGPU stack.
> 
> This effectively allows us to get rid of DRI2 and the related security 
> problems. Only thing left for that is that I'm just not sure how to 
> signal this to userspace so that the DDX wouldn't advertise DRI2 at all 
> any more.
> 
> 
> As a next step I looked into if we can disable the command submission 
> for DRM master. Turned out that this is relatively easy as well.
> 
> All we have to do is to fix the bug Michel pointed out about KMS handles 
> for display and let the DDX use a render node instead of the DRM master 
> for Glamor. Still need to sync up with Michel and/or Marek whats the 
> best way of doing this.
> 
> 
> The last step (and that's what I haven't tried yet) would be to disable 
> DRM authentication for Navi even when it is still compiled into the 
> kernel. But that is more or less just a typing exercise.

So just to get this straight, we're now full on embracing a subsystem
split here:
- amdgpu plans to ditch dri2/rendering on primary nodes
- bunch of drivers (I think more than i915 by now) merged the DRM_AUTH
  removal
- others are just hanging in there somehow

"best of both^W worlds" ftw?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Koenig, Christian]

Am 21.06.19 um 09:41 schrieb Michel Dänzer:
> On 2019-06-21 9:12 a.m., Koenig, Christian wrote:
>> First of all I tried to disable DRM authentication completely with a
>> kernel config option. Surprisingly that actually works out of the box at
>> least on the AMDGPU stack.
>>
>> This effectively allows us to get rid of DRI2 and the related security
>> problems. Only thing left for that is that I'm just not sure how to
>> signal this to userspace so that the DDX wouldn't advertise DRI2 at all
>> any more.
> FWIW, getting rid of DRI2 also needs to be discussed with amdgpu-pro
> OpenGL driver folks.

Good point, but I don't expect much problems from that direction.

IIRC they where quite happy to not have to support DRI2 except for a 
very very old X server version.

>> As a next step I looked into if we can disable the command submission
>> for DRM master. Turned out that this is relatively easy as well.
>>
>> All we have to do is to fix the bug Michel pointed out about KMS handles
>> for display
> I'm working on that, consider it fixed.
>
>
>> and let the DDX use a render node instead of the DRM master for Glamor.
>> Still need to sync up with Michel and/or Marek whats the best way of
>> doing this.
> My suggestion was to add a new variant of amdgpu_device_initialize. When
> the new variant is called, libdrm_amdgpu internally uses a render node
> for command submission etc. whenever possible.

Yeah, sounds like a plan to me.

Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Michel Dänzer]

On 2019-06-21 9:12 a.m., Koenig, Christian wrote:
> 
> First of all I tried to disable DRM authentication completely with a 
> kernel config option. Surprisingly that actually works out of the box at 
> least on the AMDGPU stack.
> 
> This effectively allows us to get rid of DRI2 and the related security 
> problems. Only thing left for that is that I'm just not sure how to 
> signal this to userspace so that the DDX wouldn't advertise DRI2 at all 
> any more.

FWIW, getting rid of DRI2 also needs to be discussed with amdgpu-pro
OpenGL driver folks.


> As a next step I looked into if we can disable the command submission 
> for DRM master. Turned out that this is relatively easy as well.
> 
> All we have to do is to fix the bug Michel pointed out about KMS handles 
> for display

I'm working on that, consider it fixed.


> and let the DDX use a render node instead of the DRM master for Glamor.
> Still need to sync up with Michel and/or Marek whats the best way of
> doing this.

My suggestion was to add a new variant of amdgpu_device_initialize. When
the new variant is called, libdrm_amdgpu internally uses a render node
for command submission etc. whenever possible.


-- 
Earthling Michel Dänzer   |  https://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Koenig, Christian]

Am 20.06.19 um 18:30 schrieb Emil Velikov:
> On 2019/06/14, Koenig, Christian wrote:
>> Am 14.06.19 um 17:53 schrieb Emil Velikov:
>>> On 2019/06/14, Koenig, Christian wrote:
 Am 14.06.19 um 14:09 schrieb Emil Velikov:
> On 2019/05/27, Emil Velikov wrote:
> [SNIP]
> Hi Christian,
>
>
> In the following, I would like to summarise and emphasize the need for
> DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
> extra reading it.
>
>
> Today DRM drivers* do not make any distinction between primary and
> render node clients.
 That is actually not 100% correct. We have a special case where a DRM
 master is allowed to change the priority of render node clients.

>>> Can you provide a link? I cannot find that code.
>> See amdgpu_sched_ioctl().
>>
> Thus for a render capable driver, any premise of
> separation, security or otherwise imposed via DRM_AUTH is a fallacy.
 Yeah, that's what I agree on. I just don't think that removing DRM_AUTH
 now is the right direction to take.

>>> Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
>>> ioctls.
>>>
>>> That aside, can you propose an alternative solution that addresses this
>>> and the second point just below?
>> Give me a few days to work on this, it's already Friday 6pm here.
>>
> Hi Christian,
>
> Any progress? As mentioned earlier, I'm OK with writing the patches although
> I would love to hear your plan.

First of all I tried to disable DRM authentication completely with a 
kernel config option. Surprisingly that actually works out of the box at 
least on the AMDGPU stack.

This effectively allows us to get rid of DRI2 and the related security 
problems. Only thing left for that is that I'm just not sure how to 
signal this to userspace so that the DDX wouldn't advertise DRI2 at all 
any more.


As a next step I looked into if we can disable the command submission 
for DRM master. Turned out that this is relatively easy as well.

All we have to do is to fix the bug Michel pointed out about KMS handles 
for display and let the DDX use a render node instead of the DRM master 
for Glamor. Still need to sync up with Michel and/or Marek whats the 
best way of doing this.


The last step (and that's what I haven't tried yet) would be to disable 
DRM authentication for Navi even when it is still compiled into the 
kernel. But that is more or less just a typing exercise.

Regards,
Christian.

>
> Thanks
> Emil

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx