[PATCH] drm/radeon: fix semaphore value init

[PATCH] drm/radeon: fix semaphore value init

[Christian König]

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
---
 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

Re: [PATCH] drm/radeon: fix semaphore value init

[Grigori Goronzy]

[-- Attachment #1.1: Type: text/plain, Size: 1432 bytes --]

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);
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/radeon: fix semaphore value init

[Christian König]

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);
>>
>

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

Re: [PATCH] drm/radeon: fix semaphore value init

[Alex Deucher]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel