[PATCH] drm/radeon/kms: consolidate GART code, fix memory fault after GPU lockup

[Jerome Glisse]

On Fri, Oct 14, 2011 at 10:14:54AM +0200, Michel Dänzer wrote:
> [ Dropping stable at kernel.org from CC, it'll get picked up for stable
> once it hits mainline ]
> 
> On Don, 2011-10-13 at 16:38 -0400, j.glisse at gmail.com wrote: 
> > From: Jerome Glisse <jglisse at redhat.com>
> > 
> > After GPU lockup VRAM gart table is unpinned and thus its pointer
> > becomes unvalid. This patch move the unpin code to a common helper
> > function and set pointer to NULL so that page update code can check
> > if it should update GPU page table or not. That way bo still bound
> > to GART can be unbound (pci_unmap_page for all there page) properly
> > while there is no need to update the GPU page table.
> > 
> > Signed-off-by: Jerome Glisse <jglisse at redhat.com>
> 
> [...]
> 
> > 			rdev->gart.pages_addr[p] = rdev->dummy_page.addr;
> >  			page_base = rdev->gart.pages_addr[p];
> >  			for (j = 0; j < (PAGE_SIZE / RADEON_GPU_PAGE_SIZE); j++, t++) {
> > -				radeon_gart_set_page(rdev, t, page_base);
> > +				if (rdev->gart.ptr) {
> > +					radeon_gart_set_page(rdev, t, page_base);
> > +				}
> >  				page_base += RADEON_GPU_PAGE_SIZE;
> >  			}
> >  		}
> > @@ -201,7 +211,9 @@ int radeon_gart_bind(struct radeon_device *rdev, unsigned offset,
> >  		rdev->gart.pages[p] = pagelist[i];
> >  		page_base = rdev->gart.pages_addr[p];
> >  		for (j = 0; j < (PAGE_SIZE / RADEON_GPU_PAGE_SIZE); j++, t++) {
> > -			radeon_gart_set_page(rdev, t, page_base);
> > +			if (rdev->gart.ptr) {
> > +				radeon_gart_set_page(rdev, t, page_base);
> > +			}
> >  			page_base += RADEON_GPU_PAGE_SIZE;
> >  		}
> >  	}
> 
> Looks like these two loops could be completely guarded by if (rdev->gart.ptr).

Will respin with that
 
> 
> > @@ -218,7 +230,9 @@ void radeon_gart_restore(struct radeon_device *rdev)
> >  	for (i = 0, t = 0; i < rdev->gart.num_cpu_pages; i++) {
> >  		page_base = rdev->gart.pages_addr[i];
> >  		for (j = 0; j < (PAGE_SIZE / RADEON_GPU_PAGE_SIZE); j++, t++) {
> > -			radeon_gart_set_page(rdev, t, page_base);
> > +			if (rdev->gart.ptr) {
> > +				radeon_gart_set_page(rdev, t, page_base);
> > +			}
> >  			page_base += RADEON_GPU_PAGE_SIZE;
> >  		}
> >  	}
> 
> I think this should rather BUG_ON(!rdev->gart.ptr).

This should only happen after GPU lockup and GPU lockup would already
print loud message so i don't think it's usefull to add another message
that would confuse user more.

Cheers,
Jerome