Re: [PATCH] drm/amdgpu: replace mutex with spin_lock (V2)

2018-06-21 Thread S, Shirish 



On 6/22/2018 9:19 AM, S, Shirish wrote:



On 6/22/2018 9:00 AM, Dave Airlie wrote:

On 31 May 2018 at 20:02, Shirish S  wrote:

mutex's lead to sleeps which should be avoided in
atomic context.
Hence this patch replaces it with the spin_locks.

Below is the stack trace:

I'm not sure I really like this series of patches, going around
replacing ATOMIC and mutex with spinlocks
isn't something that should be done lightly,

In all the patches I haven't seen what spin lock or what causes us to
be in an atomic state in the first
place and why it is necessary that we are in an atomic state for such
long sequences of code.
We have root caused the issue and reverted all the patches in this 
patch series that tend to replace

mutex's with spinlock's.

I somehow noticed that this patch did not get reverted.
Please don't merge this patch.
Regards,
Shirish S

Regards,
Shirish S

Thanks,
Dave.

BUG: sleeping function called from invalid context at 
kernel/locking/mutex.c:**

in_atomic(): 1, irqs_disabled(): 1, pid: 89, name: kworker/u4:3
CPU: 1 PID: 89 Comm: kworker/u4:3 Tainted: G    W 4.14.43 #8
Workqueue: events_unbound commit_work
Call Trace:
  dump_stack+0x4d/0x63
  ___might_sleep+0x11f/0x12e
  mutex_lock+0x20/0x42
  amdgpu_atom_execute_table+0x26/0x72
  enable_disp_power_gating_v2_1+0x85/0xae
  dce110_enable_display_power_gating+0x83/0x1b1
  dce110_power_down_fe+0x4a/0x6d
  dc_post_update_surfaces_to_stream+0x59/0x87
  amdgpu_dm_do_flip+0x239/0x298
  amdgpu_dm_commit_planes.isra.23+0x379/0x54b
  ? drm_calc_timestamping_constants+0x14b/0x15c
  amdgpu_dm_atomic_commit_tail+0x4fc/0x5d2
  ? wait_for_common+0x5b/0x69
  commit_tail+0x42/0x64
  process_one_work+0x1b0/0x314
  worker_thread+0x1cb/0x2c1
  ? create_worker+0x1da/0x1da
  kthread+0x156/0x15e
  ? kthread_flush_work+0xea/0xea
  ret_from_fork+0x22/0x40

V2: Added stack trace in commit message.

Signed-off-by: Shirish S 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +-
  drivers/gpu/drm/amd/amdgpu/atom.c    | 4 ++--
  drivers/gpu/drm/amd/amdgpu/atom.h    | 3 ++-
  3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c

index bf872f6..ba3d4b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -2033,7 +2033,7 @@ int amdgpu_atombios_init(struct amdgpu_device 
*adev)

 return -ENOMEM;
 }

- mutex_init(>mode_info.atom_context->mutex);
+ spin_lock_init(>mode_info.atom_context->lock);
 if (adev->is_atom_fw) {
 amdgpu_atomfirmware_scratch_regs_init(adev);
amdgpu_atomfirmware_allocate_fb_scratch(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c 
b/drivers/gpu/drm/amd/amdgpu/atom.c

index 69500a8..bfd98f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.c
+++ b/drivers/gpu/drm/amd/amdgpu/atom.c
@@ -1261,7 +1261,7 @@ int amdgpu_atom_execute_table(struct 
atom_context *ctx, int index, uint32_t * pa

  {
 int r;

-   mutex_lock(>mutex);
+   spin_lock(>lock);
 /* reset data block */
 ctx->data_block = 0;
 /* reset reg block */
@@ -1274,7 +1274,7 @@ int amdgpu_atom_execute_table(struct 
atom_context *ctx, int index, uint32_t * pa

 ctx->divmul[0] = 0;
 ctx->divmul[1] = 0;
 r = amdgpu_atom_execute_table_locked(ctx, index, params);
-   mutex_unlock(>mutex);
+   spin_unlock(>lock);
 return r;
  }

diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h 
b/drivers/gpu/drm/amd/amdgpu/atom.h

index a391709..54063e2 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.h
+++ b/drivers/gpu/drm/amd/amdgpu/atom.h
@@ -26,6 +26,7 @@
  #define ATOM_H

  #include 
+#include 
  #include 

  #define ATOM_BIOS_MAGIC    0xAA55
@@ -125,7 +126,7 @@ struct card_info {

  struct atom_context {
 struct card_info *card;
-   struct mutex mutex;
+   spinlock_t lock;
 void *bios;
 uint32_t cmd_table, data_table;
 uint16_t *iio;
--
2.7.4

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




--
Regards,
Shirish S

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

Re: [PATCH] drm/amdgpu: replace mutex with spin_lock (V2)

2018-06-21 Thread S, Shirish 



On 6/22/2018 9:00 AM, Dave Airlie wrote:

On 31 May 2018 at 20:02, Shirish S  wrote:

mutex's lead to sleeps which should be avoided in
atomic context.
Hence this patch replaces it with the spin_locks.

Below is the stack trace:

I'm not sure I really like this series of patches, going around
replacing ATOMIC and mutex with spinlocks
isn't something that should be done lightly,

In all the patches I haven't seen what spin lock or what causes us to
be in an atomic state in the first
place and why it is necessary that we are in an atomic state for such
long sequences of code.
We have root caused the issue and reverted all the patches in this patch 
series that tend to replace

mutex's with spinlock's.
Regards,
Shirish S

Thanks,
Dave.


BUG: sleeping function called from invalid context at kernel/locking/mutex.c:**
in_atomic(): 1, irqs_disabled(): 1, pid: 89, name: kworker/u4:3
CPU: 1 PID: 89 Comm: kworker/u4:3 Tainted: GW   4.14.43 #8
Workqueue: events_unbound commit_work
Call Trace:
  dump_stack+0x4d/0x63
  ___might_sleep+0x11f/0x12e
  mutex_lock+0x20/0x42
  amdgpu_atom_execute_table+0x26/0x72
  enable_disp_power_gating_v2_1+0x85/0xae
  dce110_enable_display_power_gating+0x83/0x1b1
  dce110_power_down_fe+0x4a/0x6d
  dc_post_update_surfaces_to_stream+0x59/0x87
  amdgpu_dm_do_flip+0x239/0x298
  amdgpu_dm_commit_planes.isra.23+0x379/0x54b
  ? drm_calc_timestamping_constants+0x14b/0x15c
  amdgpu_dm_atomic_commit_tail+0x4fc/0x5d2
  ? wait_for_common+0x5b/0x69
  commit_tail+0x42/0x64
  process_one_work+0x1b0/0x314
  worker_thread+0x1cb/0x2c1
  ? create_worker+0x1da/0x1da
  kthread+0x156/0x15e
  ? kthread_flush_work+0xea/0xea
  ret_from_fork+0x22/0x40

V2: Added stack trace in commit message.

Signed-off-by: Shirish S 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +-
  drivers/gpu/drm/amd/amdgpu/atom.c| 4 ++--
  drivers/gpu/drm/amd/amdgpu/atom.h| 3 ++-
  3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
index bf872f6..ba3d4b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -2033,7 +2033,7 @@ int amdgpu_atombios_init(struct amdgpu_device *adev)
 return -ENOMEM;
 }

-   mutex_init(>mode_info.atom_context->mutex);
+   spin_lock_init(>mode_info.atom_context->lock);
 if (adev->is_atom_fw) {
 amdgpu_atomfirmware_scratch_regs_init(adev);
 amdgpu_atomfirmware_allocate_fb_scratch(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c 
b/drivers/gpu/drm/amd/amdgpu/atom.c
index 69500a8..bfd98f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.c
+++ b/drivers/gpu/drm/amd/amdgpu/atom.c
@@ -1261,7 +1261,7 @@ int amdgpu_atom_execute_table(struct atom_context *ctx, 
int index, uint32_t * pa
  {
 int r;

-   mutex_lock(>mutex);
+   spin_lock(>lock);
 /* reset data block */
 ctx->data_block = 0;
 /* reset reg block */
@@ -1274,7 +1274,7 @@ int amdgpu_atom_execute_table(struct atom_context *ctx, 
int index, uint32_t * pa
 ctx->divmul[0] = 0;
 ctx->divmul[1] = 0;
 r = amdgpu_atom_execute_table_locked(ctx, index, params);
-   mutex_unlock(>mutex);
+   spin_unlock(>lock);
 return r;
  }

diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h 
b/drivers/gpu/drm/amd/amdgpu/atom.h
index a391709..54063e2 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.h
+++ b/drivers/gpu/drm/amd/amdgpu/atom.h
@@ -26,6 +26,7 @@
  #define ATOM_H

  #include 
+#include 
  #include 

  #define ATOM_BIOS_MAGIC0xAA55
@@ -125,7 +126,7 @@ struct card_info {

  struct atom_context {
 struct card_info *card;
-   struct mutex mutex;
+   spinlock_t lock;
 void *bios;
 uint32_t cmd_table, data_table;
 uint16_t *iio;
--
2.7.4

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


--
Regards,
Shirish S

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

Re: [PATCH] drm/amdgpu: replace mutex with spin_lock (V2)

2018-06-21 Thread Dave Airlie 
On 31 May 2018 at 20:02, Shirish S  wrote:
> mutex's lead to sleeps which should be avoided in
> atomic context.
> Hence this patch replaces it with the spin_locks.
>
> Below is the stack trace:

I'm not sure I really like this series of patches, going around
replacing ATOMIC and mutex with spinlocks
isn't something that should be done lightly,

In all the patches I haven't seen what spin lock or what causes us to
be in an atomic state in the first
place and why it is necessary that we are in an atomic state for such
long sequences of code.

Thanks,
Dave.

>
> BUG: sleeping function called from invalid context at 
> kernel/locking/mutex.c:**
> in_atomic(): 1, irqs_disabled(): 1, pid: 89, name: kworker/u4:3
> CPU: 1 PID: 89 Comm: kworker/u4:3 Tainted: GW   4.14.43 #8
> Workqueue: events_unbound commit_work
> Call Trace:
>  dump_stack+0x4d/0x63
>  ___might_sleep+0x11f/0x12e
>  mutex_lock+0x20/0x42
>  amdgpu_atom_execute_table+0x26/0x72
>  enable_disp_power_gating_v2_1+0x85/0xae
>  dce110_enable_display_power_gating+0x83/0x1b1
>  dce110_power_down_fe+0x4a/0x6d
>  dc_post_update_surfaces_to_stream+0x59/0x87
>  amdgpu_dm_do_flip+0x239/0x298
>  amdgpu_dm_commit_planes.isra.23+0x379/0x54b
>  ? drm_calc_timestamping_constants+0x14b/0x15c
>  amdgpu_dm_atomic_commit_tail+0x4fc/0x5d2
>  ? wait_for_common+0x5b/0x69
>  commit_tail+0x42/0x64
>  process_one_work+0x1b0/0x314
>  worker_thread+0x1cb/0x2c1
>  ? create_worker+0x1da/0x1da
>  kthread+0x156/0x15e
>  ? kthread_flush_work+0xea/0xea
>  ret_from_fork+0x22/0x40
>
> V2: Added stack trace in commit message.
>
> Signed-off-by: Shirish S 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +-
>  drivers/gpu/drm/amd/amdgpu/atom.c| 4 ++--
>  drivers/gpu/drm/amd/amdgpu/atom.h| 3 ++-
>  3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> index bf872f6..ba3d4b9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> @@ -2033,7 +2033,7 @@ int amdgpu_atombios_init(struct amdgpu_device *adev)
> return -ENOMEM;
> }
>
> -   mutex_init(>mode_info.atom_context->mutex);
> +   spin_lock_init(>mode_info.atom_context->lock);
> if (adev->is_atom_fw) {
> amdgpu_atomfirmware_scratch_regs_init(adev);
> amdgpu_atomfirmware_allocate_fb_scratch(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c 
> b/drivers/gpu/drm/amd/amdgpu/atom.c
> index 69500a8..bfd98f0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
> @@ -1261,7 +1261,7 @@ int amdgpu_atom_execute_table(struct atom_context *ctx, 
> int index, uint32_t * pa
>  {
> int r;
>
> -   mutex_lock(>mutex);
> +   spin_lock(>lock);
> /* reset data block */
> ctx->data_block = 0;
> /* reset reg block */
> @@ -1274,7 +1274,7 @@ int amdgpu_atom_execute_table(struct atom_context *ctx, 
> int index, uint32_t * pa
> ctx->divmul[0] = 0;
> ctx->divmul[1] = 0;
> r = amdgpu_atom_execute_table_locked(ctx, index, params);
> -   mutex_unlock(>mutex);
> +   spin_unlock(>lock);
> return r;
>  }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h 
> b/drivers/gpu/drm/amd/amdgpu/atom.h
> index a391709..54063e2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
> @@ -26,6 +26,7 @@
>  #define ATOM_H
>
>  #include 
> +#include 
>  #include 
>
>  #define ATOM_BIOS_MAGIC0xAA55
> @@ -125,7 +126,7 @@ struct card_info {
>
>  struct atom_context {
> struct card_info *card;
> -   struct mutex mutex;
> +   spinlock_t lock;
> void *bios;
> uint32_t cmd_table, data_table;
> uint16_t *iio;
> --
> 2.7.4
>
> ___
> 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

[PATCH] drm/amdgpu: replace mutex with spin_lock (V2)

2018-05-31 Thread Shirish S 
mutex's lead to sleeps which should be avoided in
atomic context.
Hence this patch replaces it with the spin_locks.

Below is the stack trace:

BUG: sleeping function called from invalid context at kernel/locking/mutex.c:**
in_atomic(): 1, irqs_disabled(): 1, pid: 89, name: kworker/u4:3
CPU: 1 PID: 89 Comm: kworker/u4:3 Tainted: GW   4.14.43 #8
Workqueue: events_unbound commit_work
Call Trace:
 dump_stack+0x4d/0x63
 ___might_sleep+0x11f/0x12e
 mutex_lock+0x20/0x42
 amdgpu_atom_execute_table+0x26/0x72
 enable_disp_power_gating_v2_1+0x85/0xae
 dce110_enable_display_power_gating+0x83/0x1b1
 dce110_power_down_fe+0x4a/0x6d
 dc_post_update_surfaces_to_stream+0x59/0x87
 amdgpu_dm_do_flip+0x239/0x298
 amdgpu_dm_commit_planes.isra.23+0x379/0x54b
 ? drm_calc_timestamping_constants+0x14b/0x15c
 amdgpu_dm_atomic_commit_tail+0x4fc/0x5d2
 ? wait_for_common+0x5b/0x69
 commit_tail+0x42/0x64
 process_one_work+0x1b0/0x314
 worker_thread+0x1cb/0x2c1
 ? create_worker+0x1da/0x1da
 kthread+0x156/0x15e
 ? kthread_flush_work+0xea/0xea
 ret_from_fork+0x22/0x40

V2: Added stack trace in commit message.

Signed-off-by: Shirish S 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/atom.c| 4 ++--
 drivers/gpu/drm/amd/amdgpu/atom.h| 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
index bf872f6..ba3d4b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -2033,7 +2033,7 @@ int amdgpu_atombios_init(struct amdgpu_device *adev)
return -ENOMEM;
}
 
-   mutex_init(>mode_info.atom_context->mutex);
+   spin_lock_init(>mode_info.atom_context->lock);
if (adev->is_atom_fw) {
amdgpu_atomfirmware_scratch_regs_init(adev);
amdgpu_atomfirmware_allocate_fb_scratch(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c 
b/drivers/gpu/drm/amd/amdgpu/atom.c
index 69500a8..bfd98f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.c
+++ b/drivers/gpu/drm/amd/amdgpu/atom.c
@@ -1261,7 +1261,7 @@ int amdgpu_atom_execute_table(struct atom_context *ctx, 
int index, uint32_t * pa
 {
int r;
 
-   mutex_lock(>mutex);
+   spin_lock(>lock);
/* reset data block */
ctx->data_block = 0;
/* reset reg block */
@@ -1274,7 +1274,7 @@ int amdgpu_atom_execute_table(struct atom_context *ctx, 
int index, uint32_t * pa
ctx->divmul[0] = 0;
ctx->divmul[1] = 0;
r = amdgpu_atom_execute_table_locked(ctx, index, params);
-   mutex_unlock(>mutex);
+   spin_unlock(>lock);
return r;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h 
b/drivers/gpu/drm/amd/amdgpu/atom.h
index a391709..54063e2 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.h
+++ b/drivers/gpu/drm/amd/amdgpu/atom.h
@@ -26,6 +26,7 @@
 #define ATOM_H
 
 #include 
+#include 
 #include 
 
 #define ATOM_BIOS_MAGIC0xAA55
@@ -125,7 +126,7 @@ struct card_info {
 
 struct atom_context {
struct card_info *card;
-   struct mutex mutex;
+   spinlock_t lock;
void *bios;
uint32_t cmd_table, data_table;
uint16_t *iio;
-- 
2.7.4

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

Re: [PATCH] drm/amdgpu: replace mutex with spin_lock

2018-05-31 Thread S, Shirish 



On 5/30/2018 9:10 PM, Christian König wrote:
Keep in mind that under SRIOV you can read registers while in atomic 
context, e.g. while holding a spinlock.


Please double check if that won't bite us.

Apart from that the change looks good to me,

Thanks Christian, i verified boot, s3, s5 on Stoney with this patch.
Have re-spun V2 which has the exact trace in which scenario this BUG is hit.
Regards,
Shirish S

Christian.

Am 30.05.2018 um 12:19 schrieb Shirish S:

mutex's lead to sleeps which should be avoided in
atomic context.
Hence this patch replaces it with the spin_locks.

Signed-off-by: Shirish S 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +-
  drivers/gpu/drm/amd/amdgpu/atom.c    | 4 ++--
  drivers/gpu/drm/amd/amdgpu/atom.h    | 3 ++-
  3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c

index bf872f6..ba3d4b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -2033,7 +2033,7 @@ int amdgpu_atombios_init(struct amdgpu_device 
*adev)

  return -ENOMEM;
  }
  - mutex_init(>mode_info.atom_context->mutex);
+ spin_lock_init(>mode_info.atom_context->lock);
  if (adev->is_atom_fw) {
  amdgpu_atomfirmware_scratch_regs_init(adev);
  amdgpu_atomfirmware_allocate_fb_scratch(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c 
b/drivers/gpu/drm/amd/amdgpu/atom.c

index 69500a8..bfd98f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.c
+++ b/drivers/gpu/drm/amd/amdgpu/atom.c
@@ -1261,7 +1261,7 @@ int amdgpu_atom_execute_table(struct 
atom_context *ctx, int index, uint32_t * pa

  {
  int r;
  -    mutex_lock(>mutex);
+    spin_lock(>lock);
  /* reset data block */
  ctx->data_block = 0;
  /* reset reg block */
@@ -1274,7 +1274,7 @@ int amdgpu_atom_execute_table(struct 
atom_context *ctx, int index, uint32_t * pa

  ctx->divmul[0] = 0;
  ctx->divmul[1] = 0;
  r = amdgpu_atom_execute_table_locked(ctx, index, params);
-    mutex_unlock(>mutex);
+    spin_unlock(>lock);
  return r;
  }
  diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h 
b/drivers/gpu/drm/amd/amdgpu/atom.h

index a391709..cdfb0d0 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.h
+++ b/drivers/gpu/drm/amd/amdgpu/atom.h
@@ -26,6 +26,7 @@
  #define ATOM_H
    #include 
+#include 
  #include 
    #define ATOM_BIOS_MAGIC    0xAA55
@@ -125,7 +126,7 @@ struct card_info {
    struct atom_context {
  struct card_info *card;
-    struct mutex mutex;
+    spinlock_t lock;
  void *bios;
  uint32_t cmd_table, data_table;
  uint16_t *iio;




--
Regards,
Shirish S

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

Re: [PATCH] drm/amdgpu: replace mutex with spin_lock

2018-05-31 Thread S, Shirish 



On 5/30/2018 8:51 PM, Deucher, Alexander wrote:

-Original Message-
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
Of Shirish S
Sent: Wednesday, May 30, 2018 6:20 AM
To: amd-gfx@lists.freedesktop.org; Wentland, Harry
; Zhu, Rex 
Cc: S, Shirish 
Subject: [PATCH] drm/amdgpu: replace mutex with spin_lock

mutex's lead to sleeps which should be avoided in atomic context.
Hence this patch replaces it with the spin_locks.

Signed-off-by: Shirish S 

Does this actually fix a bug or is it just to be safe?  Do actually call atom 
command tables in an atomic context?
I have re-spun V2 patch, with the stack trace so that you can see the 
path taken.


For reference below is the trace:

BUG: sleeping function called from invalid context at 
kernel/locking/mutex.c:238

in_atomic(): 1, irqs_disabled(): 1, pid: 89, name: kworker/u4:3
CPU: 1 PID: 89 Comm: kworker/u4:3 Tainted: G    W 4.14.43 #8
Workqueue: events_unbound commit_work
Call Trace:
 dump_stack+0x4d/0x63
 ___might_sleep+0x11f/0x12e
 mutex_lock+0x20/0x42
 amdgpu_atom_execute_table+0x26/0x72
 enable_disp_power_gating_v2_1+0x85/0xae
 dce110_enable_display_power_gating+0x83/0x1b1
 dce110_power_down_fe+0x4a/0x6d
 dc_post_update_surfaces_to_stream+0x59/0x87
 amdgpu_dm_do_flip+0x239/0x298
 amdgpu_dm_commit_planes.isra.23+0x379/0x54b
 ? drm_calc_timestamping_constants+0x14b/0x15c
 amdgpu_dm_atomic_commit_tail+0x4fc/0x5d2
 ? wait_for_common+0x5b/0x69
 commit_tail+0x42/0x64
 process_one_work+0x1b0/0x314
 worker_thread+0x1cb/0x2c1
 ? create_worker+0x1da/0x1da
 kthread+0x156/0x15e
 ? kthread_flush_work+0xea/0xea
 ret_from_fork+0x22/0x40

Its caused when a BUG hit in the kernel.

Thanks & Regards,
Shirish S

Alex


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +-
  drivers/gpu/drm/amd/amdgpu/atom.c| 4 ++--
  drivers/gpu/drm/amd/amdgpu/atom.h| 3 ++-
  3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
index bf872f6..ba3d4b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -2033,7 +2033,7 @@ int amdgpu_atombios_init(struct amdgpu_device
*adev)
return -ENOMEM;
}

-   mutex_init(>mode_info.atom_context->mutex);
+   spin_lock_init(>mode_info.atom_context->lock);
if (adev->is_atom_fw) {
amdgpu_atomfirmware_scratch_regs_init(adev);
amdgpu_atomfirmware_allocate_fb_scratch(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
b/drivers/gpu/drm/amd/amdgpu/atom.c
index 69500a8..bfd98f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.c
+++ b/drivers/gpu/drm/amd/amdgpu/atom.c
@@ -1261,7 +1261,7 @@ int amdgpu_atom_execute_table(struct
atom_context *ctx, int index, uint32_t * pa  {
int r;

-   mutex_lock(>mutex);
+   spin_lock(>lock);
/* reset data block */
ctx->data_block = 0;
/* reset reg block */
@@ -1274,7 +1274,7 @@ int amdgpu_atom_execute_table(struct
atom_context *ctx, int index, uint32_t * pa
ctx->divmul[0] = 0;
ctx->divmul[1] = 0;
r = amdgpu_atom_execute_table_locked(ctx, index, params);
-   mutex_unlock(>mutex);
+   spin_unlock(>lock);
return r;
  }

diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
b/drivers/gpu/drm/amd/amdgpu/atom.h
index a391709..cdfb0d0 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.h
+++ b/drivers/gpu/drm/amd/amdgpu/atom.h
@@ -26,6 +26,7 @@
  #define ATOM_H

  #include 
+#include 
  #include 

  #define ATOM_BIOS_MAGIC   0xAA55
@@ -125,7 +126,7 @@ struct card_info {

  struct atom_context {
struct card_info *card;
-   struct mutex mutex;
+   spinlock_t lock;
void *bios;
uint32_t cmd_table, data_table;
uint16_t *iio;
--
2.7.4

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


--
Regards,
Shirish S

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

Re: [PATCH] drm/amdgpu: replace mutex with spin_lock

2018-05-30 Thread Christian König 
Keep in mind that under SRIOV you can read registers while in atomic 
context, e.g. while holding a spinlock.


Please double check if that won't bite us.

Apart from that the change looks good to me,
Christian.

Am 30.05.2018 um 12:19 schrieb Shirish S:

mutex's lead to sleeps which should be avoided in
atomic context.
Hence this patch replaces it with the spin_locks.

Signed-off-by: Shirish S 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +-
  drivers/gpu/drm/amd/amdgpu/atom.c| 4 ++--
  drivers/gpu/drm/amd/amdgpu/atom.h| 3 ++-
  3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
index bf872f6..ba3d4b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -2033,7 +2033,7 @@ int amdgpu_atombios_init(struct amdgpu_device *adev)
return -ENOMEM;
}
  
-	mutex_init(>mode_info.atom_context->mutex);

+   spin_lock_init(>mode_info.atom_context->lock);
if (adev->is_atom_fw) {
amdgpu_atomfirmware_scratch_regs_init(adev);
amdgpu_atomfirmware_allocate_fb_scratch(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c 
b/drivers/gpu/drm/amd/amdgpu/atom.c
index 69500a8..bfd98f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.c
+++ b/drivers/gpu/drm/amd/amdgpu/atom.c
@@ -1261,7 +1261,7 @@ int amdgpu_atom_execute_table(struct atom_context *ctx, 
int index, uint32_t * pa
  {
int r;
  
-	mutex_lock(>mutex);

+   spin_lock(>lock);
/* reset data block */
ctx->data_block = 0;
/* reset reg block */
@@ -1274,7 +1274,7 @@ int amdgpu_atom_execute_table(struct atom_context *ctx, 
int index, uint32_t * pa
ctx->divmul[0] = 0;
ctx->divmul[1] = 0;
r = amdgpu_atom_execute_table_locked(ctx, index, params);
-   mutex_unlock(>mutex);
+   spin_unlock(>lock);
return r;
  }
  
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h b/drivers/gpu/drm/amd/amdgpu/atom.h

index a391709..cdfb0d0 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.h
+++ b/drivers/gpu/drm/amd/amdgpu/atom.h
@@ -26,6 +26,7 @@
  #define ATOM_H
  
  #include 

+#include 
  #include 
  
  #define ATOM_BIOS_MAGIC		0xAA55

@@ -125,7 +126,7 @@ struct card_info {
  
  struct atom_context {

struct card_info *card;
-   struct mutex mutex;
+   spinlock_t lock;
void *bios;
uint32_t cmd_table, data_table;
uint16_t *iio;


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

RE: [PATCH] drm/amdgpu: replace mutex with spin_lock

2018-05-30 Thread Deucher, Alexander 
> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Shirish S
> Sent: Wednesday, May 30, 2018 6:20 AM
> To: amd-gfx@lists.freedesktop.org; Wentland, Harry
> ; Zhu, Rex 
> Cc: S, Shirish 
> Subject: [PATCH] drm/amdgpu: replace mutex with spin_lock
> 
> mutex's lead to sleeps which should be avoided in atomic context.
> Hence this patch replaces it with the spin_locks.
> 
> Signed-off-by: Shirish S 

Does this actually fix a bug or is it just to be safe?  Do actually call atom 
command tables in an atomic context?

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +-
>  drivers/gpu/drm/amd/amdgpu/atom.c| 4 ++--
>  drivers/gpu/drm/amd/amdgpu/atom.h| 3 ++-
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> index bf872f6..ba3d4b9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> @@ -2033,7 +2033,7 @@ int amdgpu_atombios_init(struct amdgpu_device
> *adev)
>   return -ENOMEM;
>   }
> 
> - mutex_init(>mode_info.atom_context->mutex);
> + spin_lock_init(>mode_info.atom_context->lock);
>   if (adev->is_atom_fw) {
>   amdgpu_atomfirmware_scratch_regs_init(adev);
>   amdgpu_atomfirmware_allocate_fb_scratch(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
> b/drivers/gpu/drm/amd/amdgpu/atom.c
> index 69500a8..bfd98f0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
> @@ -1261,7 +1261,7 @@ int amdgpu_atom_execute_table(struct
> atom_context *ctx, int index, uint32_t * pa  {
>   int r;
> 
> - mutex_lock(>mutex);
> + spin_lock(>lock);
>   /* reset data block */
>   ctx->data_block = 0;
>   /* reset reg block */
> @@ -1274,7 +1274,7 @@ int amdgpu_atom_execute_table(struct
> atom_context *ctx, int index, uint32_t * pa
>   ctx->divmul[0] = 0;
>   ctx->divmul[1] = 0;
>   r = amdgpu_atom_execute_table_locked(ctx, index, params);
> - mutex_unlock(>mutex);
> + spin_unlock(>lock);
>   return r;
>  }
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h
> b/drivers/gpu/drm/amd/amdgpu/atom.h
> index a391709..cdfb0d0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/atom.h
> +++ b/drivers/gpu/drm/amd/amdgpu/atom.h
> @@ -26,6 +26,7 @@
>  #define ATOM_H
> 
>  #include 
> +#include 
>  #include 
> 
>  #define ATOM_BIOS_MAGIC  0xAA55
> @@ -125,7 +126,7 @@ struct card_info {
> 
>  struct atom_context {
>   struct card_info *card;
> - struct mutex mutex;
> + spinlock_t lock;
>   void *bios;
>   uint32_t cmd_table, data_table;
>   uint16_t *iio;
> --
> 2.7.4
> 
> ___
> 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

[PATCH] drm/amdgpu: replace mutex with spin_lock

2018-05-30 Thread Shirish S 
mutex's lead to sleeps which should be avoided in
atomic context.
Hence this patch replaces it with the spin_locks.

Signed-off-by: Shirish S 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/atom.c| 4 ++--
 drivers/gpu/drm/amd/amdgpu/atom.h| 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
index bf872f6..ba3d4b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -2033,7 +2033,7 @@ int amdgpu_atombios_init(struct amdgpu_device *adev)
return -ENOMEM;
}
 
-   mutex_init(>mode_info.atom_context->mutex);
+   spin_lock_init(>mode_info.atom_context->lock);
if (adev->is_atom_fw) {
amdgpu_atomfirmware_scratch_regs_init(adev);
amdgpu_atomfirmware_allocate_fb_scratch(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c 
b/drivers/gpu/drm/amd/amdgpu/atom.c
index 69500a8..bfd98f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.c
+++ b/drivers/gpu/drm/amd/amdgpu/atom.c
@@ -1261,7 +1261,7 @@ int amdgpu_atom_execute_table(struct atom_context *ctx, 
int index, uint32_t * pa
 {
int r;
 
-   mutex_lock(>mutex);
+   spin_lock(>lock);
/* reset data block */
ctx->data_block = 0;
/* reset reg block */
@@ -1274,7 +1274,7 @@ int amdgpu_atom_execute_table(struct atom_context *ctx, 
int index, uint32_t * pa
ctx->divmul[0] = 0;
ctx->divmul[1] = 0;
r = amdgpu_atom_execute_table_locked(ctx, index, params);
-   mutex_unlock(>mutex);
+   spin_unlock(>lock);
return r;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.h 
b/drivers/gpu/drm/amd/amdgpu/atom.h
index a391709..cdfb0d0 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.h
+++ b/drivers/gpu/drm/amd/amdgpu/atom.h
@@ -26,6 +26,7 @@
 #define ATOM_H
 
 #include 
+#include 
 #include 
 
 #define ATOM_BIOS_MAGIC0xAA55
@@ -125,7 +126,7 @@ struct card_info {
 
 struct atom_context {
struct card_info *card;
-   struct mutex mutex;
+   spinlock_t lock;
void *bios;
uint32_t cmd_table, data_table;
uint16_t *iio;
-- 
2.7.4

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