From owner-freebsd-x11@FreeBSD.ORG Thu Oct 30 14:32:42 2014 Return-Path: Delivered-To: x11@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id A5F6EE5E; Thu, 30 Oct 2014 14:32:42 +0000 (UTC) Received: from mailrelay008.isp.belgacom.be (mailrelay008.isp.belgacom.be [195.238.6.174]) by mx1.freebsd.org (Postfix) with ESMTP id 1ACA31FF; Thu, 30 Oct 2014 14:32:41 +0000 (UTC) X-Belgacom-Dynamic: yes X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: Al8GAMBKUlRR8nxA/2dsb2JhbABcgw6BLNUpAoEiFwEBAQEBfYQDAQEEJxMcIxALDgoJJQ8SGB4GE4gsAxYBwyINhjgBAQEBAQEBAwEBAQEejlaBcUkHhEsBBJtZghCBMoZ5hy2GaIN5PC+BB4FEAQEB Received: from 64.124-242-81.adsl-dyn.isp.belgacom.be (HELO kalimero.tijl.coosemans.org) ([81.242.124.64]) by relay.skynet.be with ESMTP; 30 Oct 2014 15:31:32 +0100 Received: from kalimero.tijl.coosemans.org (kalimero.tijl.coosemans.org [127.0.0.1]) by kalimero.tijl.coosemans.org (8.14.9/8.14.9) with ESMTP id s9UEVVfT004574; Thu, 30 Oct 2014 15:31:31 +0100 (CET) (envelope-from tijl@FreeBSD.org) Date: Thu, 30 Oct 2014 15:31:30 +0100 From: Tijl Coosemans To: Konstantin Belousov Subject: Re: [rfc] Radeon AGP support patches Message-ID: <20141030153130.4d8c441b@kalimero.tijl.coosemans.org> In-Reply-To: <20141029170329.GI53947@kib.kiev.ua> References: <20141026162442.1330d4c3@kalimero.tijl.coosemans.org> <20141027141631.GX1877@kib.kiev.ua> <20141027170055.10af15e6@kalimero.tijl.coosemans.org> <20141027162753.GB1877@kib.kiev.ua> <20141027221058.23a188d0@kalimero.tijl.coosemans.org> <20141029170329.GI53947@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: x11@FreeBSD.org, dumbbell@FreeBSD.org X-BeenThere: freebsd-x11@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: X11 on FreeBSD -- maintaining and support List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Oct 2014 14:32:42 -0000 On Wed, 29 Oct 2014 19:03:29 +0200 Konstantin Belousov wrote: > On Mon, Oct 27, 2014 at 10:10:58PM +0100, Tijl Coosemans wrote: >> On Mon, 27 Oct 2014 18:27:53 +0200 Konstantin Belousov wrote: >>> On Mon, Oct 27, 2014 at 05:00:55PM +0100, Tijl Coosemans wrote: >>>> 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? >>> >>> Yes, this approach is much better IMO. Having discriminated storage >>> for the bound pages is too ugly; >> >> New patch 1 & 3 attached. >> >>> was the whole code audited for correctness after the change ? >> >> I'm fairly confident these patches are all that's needed yes. I made >> a first implementation on Sunday afternoon. It got to the point that >> X showed a mouse pointer and background colour and then it crashed. >> It took the rest of the week to figure out why (NULL dereference in >> ttm_bo_vm_fault) and how to solve it (mark aperture range fictitious). >> It's hard to debug something without a screen. I read the code front >> to back and back to front in that time and compared it with the old DRM >> code and with the Linux DRM code. That's where patch 2 & 4 come from. > Looks good, commit. See trivial notes below. Thanks. Just to be sure, did you review 2 & 4 as well?