| Message ID | 1410084412-2541-1-git-send-email-deathsimple@vodafone.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07.09.2014 12:06, Christian König wrote: > From: Christian König <christian.koenig@amd.com> > > Semaphore values have 64 bits, not 32. This fixes a very subtle bug > that disables synchronization when the upper 32bits wasn't zero. > So essentially, half the semaphore values were never properly initialized and some loads with a lot of semaphore synchronization going on tried to use the uninitialized semaphores. I think the description in the commit could be improved according to that. I didn't get any DMA L2T copy hangs with semaphores disabled completely, but unfortunately this doesn't fix the hangs. Anyway, Reviewed-By: Grigori Goronzy <greg@chown.ath.cx> Grigori > Signed-off-by: Christian König <christian.koenig@amd.com> > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/radeon/radeon_semaphore.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_semaphore.c b/drivers/gpu/drm/radeon/radeon_semaphore.c > index 56d9fd6..abd6753 100644 > --- a/drivers/gpu/drm/radeon/radeon_semaphore.c > +++ b/drivers/gpu/drm/radeon/radeon_semaphore.c > @@ -34,7 +34,7 @@ > int radeon_semaphore_create(struct radeon_device *rdev, > struct radeon_semaphore **semaphore) > { > - uint32_t *cpu_addr; > + uint64_t *cpu_addr; > int i, r; > > *semaphore = kmalloc(sizeof(struct radeon_semaphore), GFP_KERNEL); >
Am 07.09.2014 um 19:41 schrieb Grigori Goronzy: > On 07.09.2014 12:06, Christian König wrote: >> From: Christian König <christian.koenig@amd.com> >> >> Semaphore values have 64 bits, not 32. This fixes a very subtle bug >> that disables synchronization when the upper 32bits wasn't zero. >> > So essentially, half the semaphore values were never properly > initialized and some loads with a lot of semaphore synchronization going > on tried to use the uninitialized semaphores. I think the description in > the commit could be improved according to that. Thinking about it that's actually not correct either. What we did here was allocating space for four semaphores, but only initializing the first two because the pointer didn't had the correct size. Needing more than two semaphores is rather unlikely anyway and even then we would have only messed up the syncing to UVD or VCE which wouldn't cause a lockup. I don't think we ever hit this bug in practice. > I didn't get any DMA L2T copy hangs with semaphores disabled completely, > but unfortunately this doesn't fix the hangs. As expected, I just found it while going over the code once more. My suspicion with your L2T copy problems is that we either do something wrong with the tilling setup on the DMA engines or that we still do something wrong with the VM setup, but since you don't see any VM related messages it's not very likely that it's a VM issue. We should probably try to get a bit more infos from the DMA status registers, but I'm not sure if we have released the documentation for those yet. > Anyway, > Reviewed-By: Grigori Goronzy <greg@chown.ath.cx> Thanks, Christian. > > Grigori > >> Signed-off-by: Christian König <christian.koenig@amd.com> >> Cc: stable@vger.kernel.org >> --- >> drivers/gpu/drm/radeon/radeon_semaphore.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/radeon/radeon_semaphore.c b/drivers/gpu/drm/radeon/radeon_semaphore.c >> index 56d9fd6..abd6753 100644 >> --- a/drivers/gpu/drm/radeon/radeon_semaphore.c >> +++ b/drivers/gpu/drm/radeon/radeon_semaphore.c >> @@ -34,7 +34,7 @@ >> int radeon_semaphore_create(struct radeon_device *rdev, >> struct radeon_semaphore **semaphore) >> { >> - uint32_t *cpu_addr; >> + uint64_t *cpu_addr; >> int i, r; >> >> *semaphore = kmalloc(sizeof(struct radeon_semaphore), GFP_KERNEL); >> >
On Sun, Sep 7, 2014 at 6:06 AM, Christian König <deathsimple@vodafone.de> wrote: > From: Christian König <christian.koenig@amd.com> > > Semaphore values have 64 bits, not 32. This fixes a very subtle bug > that disables synchronization when the upper 32bits wasn't zero. > > Signed-off-by: Christian König <christian.koenig@amd.com> > Cc: stable@vger.kernel.org Applied to my fixes tree. thanks! Alex > --- > drivers/gpu/drm/radeon/radeon_semaphore.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_semaphore.c b/drivers/gpu/drm/radeon/radeon_semaphore.c > index 56d9fd6..abd6753 100644 > --- a/drivers/gpu/drm/radeon/radeon_semaphore.c > +++ b/drivers/gpu/drm/radeon/radeon_semaphore.c > @@ -34,7 +34,7 @@ > int radeon_semaphore_create(struct radeon_device *rdev, > struct radeon_semaphore **semaphore) > { > - uint32_t *cpu_addr; > + uint64_t *cpu_addr; > int i, r; > > *semaphore = kmalloc(sizeof(struct radeon_semaphore), GFP_KERNEL); > -- > 1.9.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/radeon/radeon_semaphore.c b/drivers/gpu/drm/radeon/radeon_semaphore.c index 56d9fd6..abd6753 100644 --- a/drivers/gpu/drm/radeon/radeon_semaphore.c +++ b/drivers/gpu/drm/radeon/radeon_semaphore.c @@ -34,7 +34,7 @@ int radeon_semaphore_create(struct radeon_device *rdev, struct radeon_semaphore **semaphore) { - uint32_t *cpu_addr; + uint64_t *cpu_addr; int i, r; *semaphore = kmalloc(sizeof(struct radeon_semaphore), GFP_KERNEL);