From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH] drm/radeon: update comments to clarify VM setup Date: Mon, 08 Oct 2012 17:29:05 +0200 Message-ID: <5072F141.3030607@vodafone.de> References: <1349704206-14011-1-git-send-email-alexdeucher@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 06E4A9EEF9 for ; Mon, 8 Oct 2012 08:29:13 -0700 (PDT) In-Reply-To: <1349704206-14011-1-git-send-email-alexdeucher@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: alexdeucher@gmail.com Cc: Alex Deucher , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 08.10.2012 15:50, alexdeucher@gmail.com wrote: > From: Alex Deucher > > The actual set up and assignment of VM page tables > is done on the fly in radeon_gart.c. > > Signed-off-by: Alex Deucher One comment below, apart from that it's: Reviewed-by: Christian K=F6nig > --- > drivers/gpu/drm/radeon/ni.c | 4 ++++ > drivers/gpu/drm/radeon/radeon_device.c | 3 +++ > drivers/gpu/drm/radeon/si.c | 7 ++++--- > 3 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c > index 48e2337..cfb9276 100644 > --- a/drivers/gpu/drm/radeon/ni.c > +++ b/drivers/gpu/drm/radeon/ni.c > @@ -770,6 +770,10 @@ static int cayman_pcie_gart_enable(struct radeon_dev= ice *rdev) > WREG32(0x15DC, 0); > = > /* empty context1-7 */ > + /* Assign the pt base to something valid for now; the pts used for > + * the VMs are determined by the application and setup and assigned > + * on the fly in the vm part of radeon_gart.c > + */ > for (i =3D 1; i < 8; i++) { > WREG32(VM_CONTEXT0_PAGE_TABLE_START_ADDR + (i << 2), 0); > WREG32(VM_CONTEXT0_PAGE_TABLE_END_ADDR + (i << 2), 0); > diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/rad= eon/radeon_device.c > index 64a4264..1fad47e 100644 > --- a/drivers/gpu/drm/radeon/radeon_device.c > +++ b/drivers/gpu/drm/radeon/radeon_device.c > @@ -1018,6 +1018,9 @@ int radeon_device_init(struct radeon_device *rdev, > return r; > /* initialize vm here */ > mutex_init(&rdev->vm_manager.lock); > + /* FIXME start with 4G, once using 2 level pt switch to full > + * vm size space > + */ Even with 2 level pt I don't think we want to switch to the full vm size = space. Having 40bits address space minus 12bits offsets gives us 28bits for the = page directory/page table, assuming a 50/50 split between them we would = get 2 ^ 14 * 8 =3D 128kb for each page, which in my eyes is a bit much = (and that gets even worth when we get 48bits address space.....) I would rather say to make max_pfn depend on the available memory, so = that a single application can at least map all VRAM + GART memory. > rdev->vm_manager.max_pfn =3D 1 << 20; > INIT_LIST_HEAD(&rdev->vm_manager.lru_vm); > = > diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c > index c76825f..f272ead 100644 > --- a/drivers/gpu/drm/radeon/si.c > +++ b/drivers/gpu/drm/radeon/si.c > @@ -2407,12 +2407,13 @@ static int si_pcie_gart_enable(struct radeon_devi= ce *rdev) > WREG32(0x15DC, 0); > = > /* empty context1-15 */ > - /* FIXME start with 4G, once using 2 level pt switch to full > - * vm size space > - */ > /* set vm size, must be a multiple of 4 */ > WREG32(VM_CONTEXT1_PAGE_TABLE_START_ADDR, 0); > WREG32(VM_CONTEXT1_PAGE_TABLE_END_ADDR, rdev->vm_manager.max_pfn); > + /* Assign the pt base to something valid for now; the pts used for > + * the VMs are determined by the application and setup and assigned > + * on the fly in the vm part of radeon_gart.c > + */ > for (i =3D 1; i < 16; i++) { > if (i < 8) > WREG32(VM_CONTEXT0_PAGE_TABLE_BASE_ADDR + (i << 2),