Re: [PATCH] drm/amd/display: Fix reference counting for struct dc_sink.

[Mathias Fröhlich]

Good Morning,

On Thursday, 21 February 2019 22:00:40 CET Li, Sun peng (Leo) wrote:
> 
> On 2019-02-20 12:24 a.m., Mathias Fröhlich wrote:
> > Hi,
> > 
> > ping?
> > ... to the dc folks?
> > 
> > best
> > Mathias
> 
> Hi Mathias,
> 
> Sorry for the wait, change looks good to me.
> 
> Reviewed-by: Leo Li 
> ...and merged.
> 
> Thanks for cleaning this up.
> Leo
Thanks!

Mathias


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amd/display: Fix reference counting for struct dc_sink.

[Mathias Fröhlich]

Hi,

ping?
... to the dc folks?

best
Mathias

On Wednesday, 13 February 2019 21:38:03 CET Alex Deucher wrote:
> Add amd-gfx and some DC people.
> 
> Alex
> 
> On Sun, Feb 10, 2019 at 5:13 AM  wrote:
> >
> > From: Mathias Fröhlich 
> >
> > Reference counting in amdgpu_dm_connector for amdgpu_dm_connector::dc_sink
> > and amdgpu_dm_connector::dc_em_sink as well as in dc_link::local_sink seems
> > to be out of shape. Thus make reference counting consistent for these
> > members and just plain increment the reference count when the variable
> > gets assigned and decrement when the pointer is set to zero or replaced.
> > Also simplify reference counting in selected function sopes to be sure the
> > reference is released in any case. In some cases add NULL pointer check
> > before dereferencing.
> > At a hand full of places a comment is placed to stat that the reference
> > increment happened already somewhere else.
> >
> > This actually fixes the following kernel bug on my system when enabling
> > display core in amdgpu. There are some more similar bug reports around,
> > so it probably helps at more places.
> >
> >kernel BUG at mm/slub.c:294!
> >invalid opcode:  [#1] SMP PTI
> >CPU: 9 PID: 1180 Comm: Xorg Not tainted 5.0.0-rc1+ #2
> >Hardware name: Supermicro X10DAi/X10DAI, BIOS 3.0a 02/05/2018
> >RIP: 0010:__slab_free+0x1e2/0x3d0
> >Code: 8b 54 24 30 48 89 4c 24 28 e8 da fb ff ff 4c 8b 54 24 28 85 c0 0f 
> > 85 67 fe ff ff 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b 49 3b 
> > 5c 24 28 75 ab 48 8b 44 24 30 49 89 4c 24 28 49 89 44
> >RSP: 0018:b0978589fa90 EFLAGS: 00010246
> >RAX: 92f12806c400 RBX: 80200019 RCX: 92f12806c400
> >RDX: 92f12806c400 RSI: dd6421a01a00 RDI: 92ed2f406e80
> >RBP: b0978589fb40 R08: 0001 R09: c0ee4748
> >R10: 92f12806c400 R11: 0001 R12: dd6421a01a00
> >R13: 92f12806c400 R14: 92ed2f406e80 R15: dd6421a01a20
> >FS:  7f4170be0ac0() GS:92ed2fb4() 
> > knlGS:
> >CS:  0010 DS:  ES:  CR0: 80050033
> >CR2: 562818aaa000 CR3: 00045745a002 CR4: 003606e0
> >DR0:  DR1:  DR2: 
> >DR3:  DR6: fffe0ff0 DR7: 0400
> >Call Trace:
> > ? drm_dbg+0x87/0x90 [drm]
> > dc_stream_release+0x28/0x50 [amdgpu]
> > amdgpu_dm_connector_mode_valid+0xb4/0x1f0 [amdgpu]
> > drm_helper_probe_single_connector_modes+0x492/0x6b0 [drm_kms_helper]
> > drm_mode_getconnector+0x457/0x490 [drm]
> > ? drm_connector_property_set_ioctl+0x60/0x60 [drm]
> > drm_ioctl_kernel+0xa9/0xf0 [drm]
> > drm_ioctl+0x201/0x3a0 [drm]
> > ? drm_connector_property_set_ioctl+0x60/0x60 [drm]
> > amdgpu_drm_ioctl+0x49/0x80 [amdgpu]
> > do_vfs_ioctl+0xa4/0x630
> > ? __sys_recvmsg+0x83/0xa0
> > ksys_ioctl+0x60/0x90
> > __x64_sys_ioctl+0x16/0x20
> > do_syscall_64+0x5b/0x160
> > entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >RIP: 0033:0x7f417110809b
> >Code: 0f 1e fa 48 8b 05 ed bd 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff 
> > ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 
> > ff ff 73 01 c3 48 8b 0d bd bd 0c 00 f7 d8 64 89 01 48
> >RSP: 002b:7ffdd8d1c268 EFLAGS: 0246 ORIG_RAX: 0010
> >RAX: ffda RBX: 562818a8ebc0 RCX: 7f417110809b
> >RDX: 7ffdd8d1c2a0 RSI: c05064a7 RDI: 0012
> >RBP: 7ffdd8d1c2a0 R08: 562819012280 R09: 0007
> >R10:  R11: 0246 R12: c05064a7
> >R13: 0012 R14: 0012 R15: 7ffdd8d1c2a0
> >Modules linked in: nfsv4 dns_resolver nfs lockd grace fscache fuse vfat 
> > fat amdgpu intel_rapl sb_edac x86_pkg_temp_thermal intel_powerclamp 
> > coretemp kvm_intel kvm irqbypass crct10dif_pclmul chash gpu_sched 
> > crc32_pclmul snd_hda_codec_realtek ghash_clmulni_intel amd_iommu_v2 
> > iTCO_wdt iTCO_vendor_support ttm snd_hda_codec_generic snd_hda_codec_hdmi 
> > ledtrig_audio snd_hda_intel drm_kms_helper snd_hda_codec intel_cstate 
> > snd_hda_core drm snd_hwdep snd_seq snd_seq_device intel_uncore snd_pcm 
> > intel_rapl_perf snd_timer snd soundcore ioatdma pcspkr 
> > intel_wmi_thunderbolt mxm_wmi i2c_i801 lpc_ich pcc_cpufreq auth_rpcgss 
> > sunrpc igb crc32c_intel i2c_algo_bit dca wmi hid_cherry an

Re: [PATCH] drm/amd/display: Fix reference counting for struct dc_sink.

[Mathias Fröhlich]

Alex,

On Wednesday, 13 February 2019 21:38:03 CET Alex Deucher wrote:
> Add amd-gfx and some DC people.

Thanks!!
When I sent, I did not remember that there is an other list for amd!
Up to now I am much more on the MESA side ...

Mathias



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

An other radeon ring count patch

[Mathias Fröhlich]

Hi Christian, Alex,

While looking over the rest of the ring counts
I noticed an other one, this time we reserve too
much which cannot even be a potential problem.
Anyway, it's nicer to be correct.

Attached that patch to uvd_v1_0_ring_test()

Greetings

Mathias
-- next part --
A non-text attachment was scrubbed...
Name: 0001-drm-radeon-fix-count-in-uvd_v1_0_ring_test.patch
Type: text/x-patch
Size: 1035 bytes
Desc: not available
URL: 

[PATCH v2] i915: Add option to bypass vbt table.

[Mathias Fröhlich]

Hi,

On Thursday, March 01, 2012 23:23:43 Daniel Vetter wrote:
> Ah, that explains things. I've added this to the commit message and queued
> it for -next, thanks.

May be the next time I should also include the reason beyond the technical 
details in the commit message.
Thanks for queuing up!

Mathias 

Re: [PATCH v2] i915: Add option to bypass vbt table.

[Mathias Fröhlich]

Hi,

On Thursday, March 01, 2012 23:23:43 Daniel Vetter wrote:
> Ah, that explains things. I've added this to the commit message and queued
> it for -next, thanks.

May be the next time I should also include the reason beyond the technical 
details in the commit message.
Thanks for queuing up!

Mathias 


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] i915: Add option to bypass vbt table.

[Mathias Fröhlich]

Hi,

On Wednesday, February 29, 2012 22:04:45 you wrote:
> thank you for your patch. Could you please resend it as [PATCH v2] with
> the following things addressed.
Will do. Thanks!

Mathias

Re: [PATCH] i915: Add option to bypass vbt table.

[Mathias Fröhlich]

Hi,

On Wednesday, February 29, 2012 22:04:45 you wrote:
> thank you for your patch. Could you please resend it as [PATCH v2] with
> the following things addressed.
Will do. Thanks!

Mathias
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] i915: Add option to bypass vbt table.

[Mathias Fröhlich]

Hi,

Attached is a small change to i915 that fixes a customer case that I had in my 
day job.

The problem happens with an embedded board that contains vbt bios tables that 
do not match the attached display.
Using this change and the apropriate kernel boot command line they are able to 
use an otherwise completely unusable secondary display on that embedded board.

Please review.

Thanks!

Mathias

[PATCH] i915: Add option to bypass vbt table.

[Mathias Fröhlich]

Hi,

Attached is a small change to i915 that fixes a customer case that I had in my 
day job.

The problem happens with an embedded board that contains vbt bios tables that 
do not match the attached display.
Using this change and the apropriate kernel boot command line they are able to 
use an otherwise completely unusable secondary display on that embedded board.

Please review.

Thanks!

Mathias>From 4c19b0f14a69ece1bd9729bfcd86f7bbe879a0a6 Mon Sep 17 00:00:00 2001
From: Mathias Froehlich 
Date: Wed, 29 Feb 2012 20:12:59 +0100
Subject: [PATCH] i915: Add option to bypass vbt table.

Add an option to skip reading the mode entries from
the vbt bios tables.
This change enables the use of displays where the vbt table just
contains inapropriate values, but either the vesa defaults or
the video=... modes do something sensible with the attached display.

Signed-off-by: Mathias Froehlich 
---
 drivers/gpu/drm/i915/i915_drv.c   |4 ++--
 drivers/gpu/drm/i915/intel_bios.c |5 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 308f819..0975895 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -89,8 +89,8 @@ MODULE_PARM_DESC(lvds_use_ssc,
 int i915_vbt_sdvo_panel_type __read_mostly = -1;
 module_param_named(vbt_sdvo_panel_type, i915_vbt_sdvo_panel_type, int, 0600);
 MODULE_PARM_DESC(vbt_sdvo_panel_type,
-		"Override selection of SDVO panel mode in the VBT "
-		"(default: auto)");
+		"Override/Ignore selection of SDVO panel mode in the VBT "
+		"(-2=ignore, -1=auto [default], index in VBT BIOS table)");
 
 static bool i915_try_reset __read_mostly = true;
 module_param_named(reset, i915_try_reset, bool, 0600);
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 63880e2..7670b09 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -255,6 +255,11 @@ parse_sdvo_panel_data(struct drm_i915_private *dev_priv,
 	int index;
 
 	index = i915_vbt_sdvo_panel_type;
+	if (index == -2) {
+		DRM_DEBUG_KMS("Ignore SDVO panel mode from BIOS VBT tables.\n");
+		return;
+	}
+
 	if (index == -1) {
 		struct bdb_sdvo_lvds_options *sdvo_lvds_options;
 
-- 
1.7.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/radeon: move ring syncing after bo validation

[Mathias Fröhlich]

Christian,

On Thursday, February 23, 2012 15:18:42 Christian K?nig wrote:
> The function radeon_bo_list_validate can cause a
> bo to move, resulting in a different sync_obj
> and a dependency to wait for this move to finish.
> 
> Signed-off-by: Christian K?nig 
> Reviewed-by: Alex Deucher 

I am not sure, but to me this looks like this could fix these kind of gpu 
lockups that I experience since some time every now and then.
The usual symptom is that I get the 

radeon :01:00.0: GPU lockup CP stall for more than 1msec
GPU lockup (waiting for 0x00682AC3 last fence id 0x00682AC2)
[...]

kernel message. Each time with the fence being off by one like in the example 
above.

If this change has the potential to fix this issue I think this particular 
patch should be considered for the current upstream kernel release.

Mathias

Re: [PATCH 1/4] drm/radeon: move ring syncing after bo validation

[Mathias Fröhlich]

Christian,

On Thursday, February 23, 2012 15:18:42 Christian König wrote:
> The function radeon_bo_list_validate can cause a
> bo to move, resulting in a different sync_obj
> and a dependency to wait for this move to finish.
> 
> Signed-off-by: Christian König 
> Reviewed-by: Alex Deucher 

I am not sure, but to me this looks like this could fix these kind of gpu 
lockups that I experience since some time every now and then.
The usual symptom is that I get the 

radeon :01:00.0: GPU lockup CP stall for more than 1msec
GPU lockup (waiting for 0x00682AC3 last fence id 0x00682AC2)
[...]

kernel message. Each time with the fence being off by one like in the example 
above.

If this change has the potential to fix this issue I think this particular 
patch should be considered for the current upstream kernel release.

Mathias
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC] Avoid quadratic behavior in relocs/cs

[Mathias Fröhlich]

Hi,

Attached is a change to the radeon reloc emitting code, that stores the reloc 
index in the buffer object. This avoids quadratic runtime behavior in the 
number of emitted buffer object relocs per command stream.
The reloc index is held in an array indexed by command stream number, which 
means that it should be safe to use with multiple command stream objects in 
place.

The patch itself is bigger than needed since it reindents the 'buffer object 
already has an index' case in cs_gem_write_reloc.

This change is driven by cs_gem_write_reloc already showing up noticable in 
profiles of some of my use cases.

Comments?

By the way, it appears to me that space_accounted should be also a per command 
stream member of the buffer object. True?

Greetings

Mathias
-- next part --
A non-text attachment was scrubbed...
Name: reloc-index-search.diff
Type: text/x-patch
Size: 9911 bytes
Desc: not available
URL: 

[RFC] Avoid quadratic behavior in relocs/cs

[Mathias Fröhlich]

Hi,

Attached is a change to the radeon reloc emitting code, that stores the reloc 
index in the buffer object. This avoids quadratic runtime behavior in the 
number of emitted buffer object relocs per command stream.
The reloc index is held in an array indexed by command stream number, which 
means that it should be safe to use with multiple command stream objects in 
place.

The patch itself is bigger than needed since it reindents the 'buffer object 
already has an index' case in cs_gem_write_reloc.

This change is driven by cs_gem_write_reloc already showing up noticable in 
profiles of some of my use cases.

Comments?

By the way, it appears to me that space_accounted should be also a per command 
stream member of the buffer object. True?

Greetings

Mathias
diff --git a/radeon/radeon_bo_gem.c b/radeon/radeon_bo_gem.c
index 081ccb9..6f0a1b7 100644
--- a/radeon/radeon_bo_gem.c
+++ b/radeon/radeon_bo_gem.c
@@ -39,7 +39,6 @@
 #include 
 #include 
 #include "xf86drm.h"
-#include "xf86atomic.h"
 #include "drm.h"
 #include "radeon_drm.h"
 #include "radeon_bo.h"
@@ -50,7 +49,6 @@ struct radeon_bo_gem {
 struct radeon_bo_int base;
 uint32_tname;
 int map_count;
-atomic_treloc_in_cs;
 void *priv_ptr;
 };
 
@@ -68,7 +66,7 @@ static struct radeon_bo *bo_open(struct radeon_bo_manager *bom,
  uint32_t flags)
 {
 struct radeon_bo_gem *bo;
-int r;
+int i, r;
 
 bo = (struct radeon_bo_gem*)calloc(1, sizeof(struct radeon_bo_gem));
 if (bo == NULL) {
@@ -82,7 +80,8 @@ static struct radeon_bo *bo_open(struct radeon_bo_manager *bom,
 bo->base.domains = domains;
 bo->base.flags = flags;
 bo->base.ptr = NULL;
-atomic_set(&bo->reloc_in_cs, 0);
+for (i = 0; i < sizeof(bo->base.idx_by_cs)/sizeof(bo->base.idx_by_cs[0]); ++i)
+bo->base.idx_by_cs[i] = -1;
 bo->map_count = 0;
 if (handle) {
 struct drm_gem_open open_arg;
@@ -312,12 +311,6 @@ uint32_t radeon_gem_name_bo(struct radeon_bo *bo)
 return bo_gem->name;
 }
 
-void *radeon_gem_get_reloc_in_cs(struct radeon_bo *bo)
-{
-struct radeon_bo_gem *bo_gem = (struct radeon_bo_gem*)bo;
-return &bo_gem->reloc_in_cs;
-}
-
 int radeon_gem_get_kernel_name(struct radeon_bo *bo, uint32_t *name)
 {
 struct radeon_bo_int *boi = (struct radeon_bo_int *)bo;
diff --git a/radeon/radeon_bo_gem.h b/radeon/radeon_bo_gem.h
index 0af8610..c56c58e 100644
--- a/radeon/radeon_bo_gem.h
+++ b/radeon/radeon_bo_gem.h
@@ -38,7 +38,6 @@ struct radeon_bo_manager *radeon_bo_manager_gem_ctor(int fd);
 void radeon_bo_manager_gem_dtor(struct radeon_bo_manager *bom);
 
 uint32_t radeon_gem_name_bo(struct radeon_bo *bo);
-void *radeon_gem_get_reloc_in_cs(struct radeon_bo *bo);
 int radeon_gem_set_domain(struct radeon_bo *bo, uint32_t read_domains, uint32_t write_domain);
 int radeon_gem_get_kernel_name(struct radeon_bo *bo, uint32_t *name);
 #endif
diff --git a/radeon/radeon_bo_int.h b/radeon/radeon_bo_int.h
index 9589ead..f47f790 100644
--- a/radeon/radeon_bo_int.h
+++ b/radeon/radeon_bo_int.h
@@ -6,6 +6,8 @@ struct radeon_bo_manager {
 int fd;
 };
 
+#define MAX_CS_COUNT 32
+
 struct radeon_bo_int {
 void*ptr;
 uint32_tflags;
@@ -18,6 +20,8 @@ struct radeon_bo_int {
 struct radeon_bo_manager*bom;
 uint32_tspace_accounted;
 uint32_treferenced_in_cs;
+/* store the reloc index indexed by the cs index number */
+uint32_tidx_by_cs[MAX_CS_COUNT];
 };
 
 /* bo functions */
diff --git a/radeon/radeon_cs_gem.c b/radeon/radeon_cs_gem.c
index 81bd393..00440e6 100644
--- a/radeon/radeon_cs_gem.c
+++ b/radeon/radeon_cs_gem.c
@@ -43,7 +43,6 @@
 #include "radeon_bo_gem.h"
 #include "drm.h"
 #include "xf86drm.h"
-#include "xf86atomic.h"
 #include "radeon_drm.h"
 #include "bof.h"
 
@@ -94,6 +93,9 @@ static uint32_t get_first_zero(const uint32_t n)
 static uint32_t generate_id(void)
 {
 uint32_t r = 0;
+
+assert(MAX_CS_COUNT == 8*sizeof(r));
+
 pthread_mutex_lock( &id_mutex );
 /* check for free ids */
 if (cs_id_source != ~r) {
@@ -142,6 +144,7 @@ static struct radeon_cs_int *cs_gem_create(struct radeon_cs_manager *csm,
 csg->base.relocs_total_size = 0;
 csg->base.crelocs = 0;
 csg->base.id = generate_id();
+csg->base.index = get_first_zero(csg->base.id) - 2;
 csg->nrelocs = 4096 / (4 * 4) ;
 csg->relocs_bo = (struct radeon_bo_int**)calloc(1,
 csg->nrelocs*sizeof(void*));
@@ -176,7 +179,6 @@ static int cs_gem_write_reloc(struct radeon_cs_int *cs,
 struct cs_gem *csg = (struct cs_gem*)cs;
 struct cs_reloc_gem *reloc;
 uint32_t idx;
-unsigned i;
 
 assert(boi->space_accounted);
 
@@ -193,46 +195,38 @@ static int cs_gem_write_reloc(struct radeon_cs_int *cs,
 if (write_domain == RADEON_GEM_DOM