Re: r259580 breaks radeonkms

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/08/14 19:38, Konstantin Belousov wrote:
> On Wed, Aug 06, 2014 at 06:52:14PM +0200, Roger Pau Monn?? wrote:
>> On 06/08/14 16:35, Nathan Whitehorn wrote:
>>> 
>>> 
>>> On 2014-08-06 02:35, Roger Pau Monn?? wrote:
>>>> On 06/08/14 02:37, Nathan Whitehorn wrote:
>>>>> Kernels with r269580 will panic when loading the radeonkms
>>>>> driver in pmap_page_set_memattr(). This probably indicates
>>>>> a bug in radeonkms, but the system is unusable in the
>>>>> meantime. -Nathan
>>>> 
>>>> I seem to be able to load radeonkms just fine after r269580:
>>> 
>>> It's possible that it is related to actually using it, rather
>>> than loading the module. I was only testing them together. I'm
>>> using vt and the panic (page fault in kernel mode) occurs when
>>> TTM tries to set memory attributes on some page while starting
>>> X. Before the panic, I see all the normal Radeon module
>>> messages as you do, so the module seems to have actually loaded
>>> correctly.  The KMS console also seems to be functional enough
>>> to display the panic message, so I suspect it's X that triggers
>>> it. -Nathan
>> 
>> Please try the attached patch, it seems to solve the panic for
>> me. It also includes a fix for Intel i915 gem, which I'm not able
>> to test because I don't have the hardware.
>> 
>> Roger.
>> 
> 
>> From 9dd3a21d99ff2fc7bf3299359751d2399eee912a Mon Sep 17 00:00:00
>> 2001 From: Roger Pau Monne <roger.pau_at_citrix.com> Date: Wed, 6
>> Aug 2014 18:16:53 +0200 Subject: [PATCH] drm: fix usage of
>> vm_phys_fictitious_to_vm_page
>> 
>> vm_phys_fictitious_to_vm_page should not be called directly, even
>> when operating on a range that has been registered using 
>> vm_phys_fictitious_reg_range. PHYS_TO_VM_PAGE should be used
>> instead because on arches that use VM_PHYSSEG_DENSE the page
>> might come directly from vm_page_array.
>> 
>> Reported by: nwhitehorn Sponsored by: Citrix Systems R&D --- 
>> sys/dev/drm2/i915/i915_gem.c |    6 ++++-- 
>> sys/dev/drm2/ttm/ttm_bo_vm.c |    8 ++++++-- 2 files changed, 10
>> insertions(+), 4 deletions(-)
>> 
>> diff --git a/sys/dev/drm2/i915/i915_gem.c
>> b/sys/dev/drm2/i915/i915_gem.c index a3acb60..6d46207 100644 ---
>> a/sys/dev/drm2/i915/i915_gem.c +++
>> b/sys/dev/drm2/i915/i915_gem.c _at__at_ -1428,8 +1428,10 _at__at_ retry:
>> 
>> obj->fault_mappable = true; VM_OBJECT_WLOCK(vm_obj); -	m =
>> vm_phys_fictitious_to_vm_page(dev->agp->base + obj->gtt_offset + 
>> -	    offset); +	m = PHYS_TO_VM_PAGE(dev->agp->base +
>> obj->gtt_offset + offset); +	KASSERT((m->flags & PG_FICTITIOUS)
>> != 0, +	    ("physical address %#jx not fictitious", +
>> (uintmax_t)(dev->agp->base + obj->gtt_offset + offset))); if (m
>> == NULL) { VM_OBJECT_WUNLOCK(vm_obj); cause = 60; diff --git
>> a/sys/dev/drm2/ttm/ttm_bo_vm.c b/sys/dev/drm2/ttm/ttm_bo_vm.c 
>> index 83ec76c..7aa1ac0 100644 --- a/sys/dev/drm2/ttm/ttm_bo_vm.c 
>> +++ b/sys/dev/drm2/ttm/ttm_bo_vm.c _at__at_ -216,8 +216,12 _at__at_ reserve: 
>> }
>> 
>> if (bo->mem.bus.is_iomem) { -		m =
>> vm_phys_fictitious_to_vm_page(bo->mem.bus.base + -
>> bo->mem.bus.offset + offset); +		m =
>> PHYS_TO_VM_PAGE(bo->mem.bus.base + bo->mem.bus.offset + +
>> offset); +		KASSERT((m->flags & PG_FICTITIOUS) != 0, +
>> ("physical address %#jx not fictitious", +
>> (uintmax_t)(bo->mem.bus.base + bo->mem.bus.offset +		    +
>> offset))); pmap_page_set_memattr(m,
>> ttm_io_prot(bo->mem.placement)); } else { ttm = bo->ttm;
> 
> This should be fine.  In fact I considered doing similar change
> some time ago, but for some reasons decided not to.  Still, it is
> not clear why does it break with your changes, in particular, the
> PCI hole where the aperture is mapped should be covered by
> vm_page_array.

This is because I've changed the logic in
vm_phys_fictitious_reg_range, so that if a region is covered by
vm_page_array it is no longer added to the red-black tree that tracks
fictitious ranges, and thus vm_phys_fictitious_to_vm_page returns NULL
in those cases.

Roger.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Darwin)

iQEcBAEBAgAGBQJT4mmWAAoJEKXZdqUyumTAEpIH/1c/BgcC0MvlTPTq4yamnGTD
FdoaNABSfh6VRRfAlQjzyTKldVLpXfcZnTpVaHiTNZgT2TRDzFoI3Brawhr33grg
1MDpYS+EusxadkTBYrsV8rQ4+t1K6jCPxgOJPVnB+85ud2Uu6SWJgilfZTW2Raq4
AKlItP2BrKtH0+Fbrtg7rBsz/Knx7kExC7bPHyBBDDuakfZJFhfIn/To1iJgw5Ug
Nmtje8IhEChtZFG9Q3S1IT8Azb8FlnImXaBdwk2bBhjMaeP0jKQFV7a6lpG+4Jjs
3/PeJtgsQpCry7oIZNZ8sCptQSRAx4lSKjgpqoTchowSv1Fj41a0s7IsJ4ALiQ8=
=DbvG
-----END PGP SIGNATURE-----