[rfc] Radeon AGP support patches

[Tijl Coosemans]

On Mon, 27 Oct 2014 16:16:31 +0200 Konstantin Belousov <kostikbel at gmail.com> wrote:
> On Sun, Oct 26, 2014 at 04:24:42PM +0100, Tijl Coosemans wrote:
>> I worked on AGP support for Radeon cards this week.  Please take a look
>> at the attached patches.
>> 
>> Patch 1:
>> Adds support for AGP_USER_TYPES to sys/dev/agp.  For normal memory types
>> a vm_object is allocated, for user types only a vm_page array.  It is
>> then up to the caller (e.g. TTM code) to manage this array.  Arbitrary
>> pages can be mapped into the GTT this way.
>>
>> Patch 3:
>> Enable AGP support in sys/dev/drm2.  In PCI mode the GTT exists on the
>> graphics card so when accessing system memory it already does its own
>> virtual address translation and only physical addresses appear on the
>> system bus.  The CPU can access the same addresses with its own VM
>> system like it always does.  In AGP mode, translation is done by the
>> AGP chipset so fictitious addresses appear on the system bus.  For the
>> CPU cache management to work correctly it needs to use these same
>> fictitious addresses instead of using the real physical addresses
>> directly.  The patch marks the AGP aperture range fictitious in
>> radeon_device.c where the VRAM aperture is also marked fictitious such
>> that PHYS_TO_VM_PAGE in ttm_bo_vm_fault works for addresses in this
>> range.
>> 
>> The rest of the patch is mostly porting to our agp_* API.  It also
>> fixes two memory leaks in ttm_agp_backend.c.  One is a missing free in
>> ttm_agp_tt_create.  The other is because ttm_agp_bind allocates an
>> agp_memory struct but ttm_agp_unbind does not free it.  So when calling
>> ttm_agp_bind a second time the reference to the struct is lost.  The
>> patch changes ttm_agp_bind so the allocation only happens in the first
>> call.  The struct is released in ttm_agp_destroy.
> Looking at the combination of patch 1 + 3.
> 
> Do you really need to change the container for the AGP_USER_MEMORY ?
> Wouldn't it be enough to allocate array in ttm_agp_bind() and copy
> pointers to pages from the agp backing object to the array ?

In ttm_agp_bind the ttm->pages array is already populated.  These are
the pages that need to be put into the GTT.  The patch modifies struct
agp_memory in sys/dev/agp such that ttm->pages can be passed via
agp_bind_memory.  Maybe it would be better to add two new functions to
sys/dev/agp/agp.c: agp_bind_pages and agp_unbind_pages.  These would
take a vm_page_t array as argument and bind/unbind the pages directly
in the GTT.  There's no need for ttm_agp_bind to call agp_alloc_memory
then and struct agp_memory would not be involved at all.  Does that
sound better?