Re: amdgpu: Corrupted video on 32 bit systems (possible fix)

Re: amdgpu: Corrupted video on 32 bit systems (possible fix)

[Michel Dänzer]

On 20/01/17 04:35 AM, Nils Holland wrote:
> Hi folks,
> 
> there seems to be an issue on 32 bit kernels which makes graphics
> output look all messed up under X when using the amdgpu drm kernel
> driver.
> 
> In fact, the same issue was present at some time in 2015 using the
> radeon driver too, but it has been fixed a long time ago, as can be
> seen here:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=84627#c35
> 
> Now, I reported the same issue in conjunction with the amdgpu driver
> back when I first noticed it back then. Nothing has happened since
> then, but my bug report can still be found here:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=91831
> 
> Personally, I had been using the (working) radeon driver again, and
> only today I decided to check if the issue in amdgpu still exists. And
> yes, it seems so: I still get garbled output when using a current
> kernel with a current version of the amdgpu driver.
> 
> Now, I've tried to just "port"  the fix that had been done to the radeon
> driver in 2015 (see the first link above) to the amdgpu driver and created
> myself the following little patch:
> 
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c	2016-12-11 20:17:54.000000000 +0100
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c	2017-01-19 15:38:56.972034489 +0100
> @@ -372,6 +372,10 @@
>  	if (!drm_arch_can_wc_memory())
>  		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>  
> +	#ifdef CONFIG_X86_32
> +		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
> +	#endif
> +
>  	amdgpu_fill_placement_to_bo(bo, placement);
>  	/* Kernel allocation are uninterruptible */
>  	r = ttm_bo_init(&adev->mman.bdev, &bo->tbo, size, type,

The corresponding code in the radeon driver has changed quite a bit
since this original fix. It would be better to bring the amdgpu code in
line with the current radeon code.


> With this patch, the amdgpu driver works fine for me on my 32 bit
> kernel: All graphics output looks the way it's supposed to, even with
> full acceleration enabled - great!
> 
> I'd suggest that it might be a good idea to put to apply the above
> patch or something similar to the official sources.

Indeed. Do you want to create a proper patch and submit it to the
amd-gfx mailing list for review? See Documentation/SubmittingPatches for
more information.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: amdgpu: Corrupted video on 32 bit systems (possible fix)

[Nils Holland]

On Fri, Jan 20, 2017 at 11:47:53AM +0900, Michel Dänzer wrote:
> On 20/01/17 04:35 AM, Nils Holland wrote:
> > 
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c	2016-12-11 20:17:54.000000000 +0100
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c	2017-01-19 15:38:56.972034489 +0100
> > @@ -372,6 +372,10 @@
> >  	if (!drm_arch_can_wc_memory())
> >  		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
> >  
> > +	#ifdef CONFIG_X86_32
> > +		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
> > +	#endif
> > +
> >  	amdgpu_fill_placement_to_bo(bo, placement);
> >  	/* Kernel allocation are uninterruptible */
> >  	r = ttm_bo_init(&adev->mman.bdev, &bo->tbo, size, type,
> 
> The corresponding code in the radeon driver has changed quite a bit
> since this original fix. It would be better to bring the amdgpu code in
> line with the current radeon code.
> 
> 
> > With this patch, the amdgpu driver works fine for me on my 32 bit
> > kernel: All graphics output looks the way it's supposed to, even with
> > full acceleration enabled - great!
> > 
> > I'd suggest that it might be a good idea to put to apply the above
> > patch or something similar to the official sources.
> 
> Indeed. Do you want to create a proper patch and submit it to the
> amd-gfx mailing list for review? See Documentation/SubmittingPatches for
> more information.

Sounds like a good idea! I was a bit heasitant because, to be honest,
I'm not at all an expert about the code in question and basically only
saw how you fixed the issue in radeon and thought: "Well, let's see if
I can do the same thing in amdgpu and if so, if it helps there, too".
;-)

However, since you've said that a 32 bit fix in amdgpu generally seems
like a good idea, I would indeed use a little time on the weekend to
get a proper patch ready and submit it for review. Even if the "no wc
for x86_32" part is probably the only thing it'll contain - more of
"bringing the amdgpu code in line with the current radeon code" might,
for the time being, be beyond my capabilities, at least if we assume
that the code should stay in a sane and working condition. ;-)

Thanks and greetings
Nils

Re: amdgpu: Corrupted video on 32 bit systems (possible fix)

[Christian König]

Am 20.01.2017 um 08:44 schrieb Nils Holland:
> On Fri, Jan 20, 2017 at 11:47:53AM +0900, Michel Dänzer wrote:
>> On 20/01/17 04:35 AM, Nils Holland wrote:
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c	2016-12-11 20:17:54.000000000 +0100
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c	2017-01-19 15:38:56.972034489 +0100
>>> @@ -372,6 +372,10 @@
>>>   	if (!drm_arch_can_wc_memory())
>>>   		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>>   
>>> +	#ifdef CONFIG_X86_32
>>> +		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>> +	#endif
>>> +
>>>   	amdgpu_fill_placement_to_bo(bo, placement);
>>>   	/* Kernel allocation are uninterruptible */
>>>   	r = ttm_bo_init(&adev->mman.bdev, &bo->tbo, size, type,
>> The corresponding code in the radeon driver has changed quite a bit
>> since this original fix. It would be better to bring the amdgpu code in
>> line with the current radeon code.
>>
>>
>>> With this patch, the amdgpu driver works fine for me on my 32 bit
>>> kernel: All graphics output looks the way it's supposed to, even with
>>> full acceleration enabled - great!
>>>
>>> I'd suggest that it might be a good idea to put to apply the above
>>> patch or something similar to the official sources.
>> Indeed. Do you want to create a proper patch and submit it to the
>> amd-gfx mailing list for review? See Documentation/SubmittingPatches for
>> more information.
> Sounds like a good idea! I was a bit heasitant because, to be honest,
> I'm not at all an expert about the code in question and basically only
> saw how you fixed the issue in radeon and thought: "Well, let's see if
> I can do the same thing in amdgpu and if so, if it helps there, too".
> ;-)

Well first of all thanks for testing this and pointing into the correct 
direction how to fix the issue.

>
> However, since you've said that a 32 bit fix in amdgpu generally seems
> like a good idea, I would indeed use a little time on the weekend to
> get a proper patch ready and submit it for review. Even if the "no wc
> for x86_32" part is probably the only thing it'll contain - more of
> "bringing the amdgpu code in line with the current radeon code" might,
> for the time being, be beyond my capabilities, at least if we assume
> that the code should stay in a sane and working condition. ;-)

The amdgpu code is a successor of the radeon code, we just forked it of 
to avoid regressing radeon while working on new stuff (and radeon just 
became way to big to handle).

Unfortunately that fork happened before the final fix in radeon for 
32bit systems landed. I think it's just the #if condition which changed 
on radeon, e.g. instead of always disabling it on 32bit X86 we do so 
when the feature needed for USWC isn't enabled on the architecture in 
general IIRC.

Shouldn't be to much of an issue to port the current status over to amdgpu.

Regards,
Christian.

>
> Thanks and greetings
> Nils
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: amdgpu: Corrupted video on 32 bit systems (possible fix)

[Michel Dänzer]

On 20/01/17 04:44 PM, Nils Holland wrote:
> On Fri, Jan 20, 2017 at 11:47:53AM +0900, Michel Dänzer wrote:
>> On 20/01/17 04:35 AM, Nils Holland wrote:
>>>
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c	2016-12-11 20:17:54.000000000 +0100
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c	2017-01-19 15:38:56.972034489 +0100
>>> @@ -372,6 +372,10 @@
>>>  	if (!drm_arch_can_wc_memory())
>>>  		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>>  
>>> +	#ifdef CONFIG_X86_32
>>> +		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>> +	#endif
>>> +
>>>  	amdgpu_fill_placement_to_bo(bo, placement);
>>>  	/* Kernel allocation are uninterruptible */
>>>  	r = ttm_bo_init(&adev->mman.bdev, &bo->tbo, size, type,
>>
>> The corresponding code in the radeon driver has changed quite a bit
>> since this original fix. It would be better to bring the amdgpu code in
>> line with the current radeon code.
>>
>>
>>> With this patch, the amdgpu driver works fine for me on my 32 bit
>>> kernel: All graphics output looks the way it's supposed to, even with
>>> full acceleration enabled - great!
>>>
>>> I'd suggest that it might be a good idea to put to apply the above
>>> patch or something similar to the official sources.
>>
>> Indeed. Do you want to create a proper patch and submit it to the
>> amd-gfx mailing list for review? See Documentation/SubmittingPatches for
>> more information.
> 
> Sounds like a good idea! I was a bit heasitant because, to be honest,
> I'm not at all an expert about the code in question and basically only
> saw how you fixed the issue in radeon and thought: "Well, let's see if
> I can do the same thing in amdgpu and if so, if it helps there, too".
> ;-)
> 
> However, since you've said that a 32 bit fix in amdgpu generally seems
> like a good idea,

Actually, unless your CPU can't run 64-bit code, I'd say running a
64-bit kernel would be an overall even better idea for you, even with
32-bit userspace. :) Anyway, this problem clearly needs to be fixed.


> I would indeed use a little time on the weekend to get a proper patch
> ready and submit it for review. Even if the "no wc for x86_32" part is
> probably the only thing it'll contain

I wouldn't bother with that. There is no real reason against bringing it
all over in one go.

> - more of "bringing the amdgpu code in line with the current radeon
> code" might, for the time being, be beyond my capabilities, at least
> if we assume that the code should stay in a sane and working
> condition. ;-)

Don't worry, it's mostly a matter of copy'n'paste, testing on your end,
review and more testing by others. :)


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

Re: amdgpu: Corrupted video on 32 bit systems (possible fix)

[Nils Holland]

On Fri, Jan 20, 2017 at 06:11:50PM +0900, Michel Dänzer wrote:
> On 20/01/17 04:44 PM, Nils Holland wrote:
> > 
> > Sounds like a good idea! I was a bit heasitant because, to be honest,
> > I'm not at all an expert about the code in question and basically only
> > saw how you fixed the issue in radeon and thought: "Well, let's see if
> > I can do the same thing in amdgpu and if so, if it helps there, too".
> > ;-)
> > 
> > However, since you've said that a 32 bit fix in amdgpu generally seems
> > like a good idea,
> 
> Actually, unless your CPU can't run 64-bit code, I'd say running a
> 64-bit kernel would be an overall even better idea for you, even with
> 32-bit userspace. :) Anyway, this problem clearly needs to be fixed.

Yep, that's certainly true! However, I have to admit that I'm
intentionally regularly testing all kinds of stuff on pure 32 bit,
because I think that most of the other "first like of testing", i.e.
the developers themselves, are primarily working with 64 bit these
days. So I thought it might make sense if someone keeps a little of an
eye on 32 bit - after all, in general and if there are no technical
reasons that speak agsinst it, things should work there, too. And
indeed, during the past year I've spotted a few little things that
were only causing problems on 32 bit - both in the kernel and other
components like systemd - and which could subsequently be fixed in a
relatively easy manner.

> > I would indeed use a little time on the weekend to get a proper patch
> > ready and submit it for review. Even if the "no wc for x86_32" part is
> > probably the only thing it'll contain
> 
> I wouldn't bother with that. There is no real reason against bringing it
> all over in one go.

Right, I'll see what I can do over the weekend. So stay tuned for my
first ever proper (at least as far as the guidelines for submitting
patches are concerned) patch and expect the worst. ;-)

Greetings
Nils
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx