Re: [PATCH] drm/radeon/kms: forbid allocating bo bigger than VRAM or GTT (fdo 31708)

From: "Michel Dänzer" <michel@daenzer.net>
To: jglisse@redhat.com
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/radeon/kms: forbid allocating bo bigger than VRAM or GTT (fdo 31708)
Date: Fri, 19 Nov 2010 18:56:27 +0100	[thread overview]
Message-ID: <1290189387.4130.133.camel@thor.local> (raw)
In-Reply-To: <1290106335-4249-1-git-send-email-jglisse@redhat.com>

On Don, 2010-11-18 at 13:52 -0500, jglisse@redhat.com wrote: 
> From: Jerome Glisse <jglisse@redhat.com>
> 
> Forbid allocating buffer bigger than VRAM or GTT, also properly set
> lpfn field of placement if VRAM is too small.
> 
> Signed-off-by: Jerome Glisse <jglisse@redhat.com>

[...]

> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
> index 8eb1834..a09d076 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.c
> +++ b/drivers/gpu/drm/radeon/radeon_object.c
> @@ -64,12 +64,18 @@ bool radeon_ttm_bo_is_radeon_bo(struct ttm_buffer_object *bo)
>  	return false;
>  }
>  
> -void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain)
> +void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain,  u32 size)
>  {
>  	u32 c = 0;
>  
>  	rbo->placement.fpfn = 0;
>  	rbo->placement.lpfn = rbo->rdev->mc.active_vram_size >> PAGE_SHIFT;
> +	/* size bigger than vram directly fallback to GTT*/
> +	if (size >= rbo->rdev->mc.active_vram_size) {
> +		rbo->placement.lpfn = rbo->rdev->mc.gtt_size >> PAGE_SHIFT;
> +		if (!(domain & (RADEON_GEM_DOMAIN_GTT | RADEON_GEM_DOMAIN_CPU)))
> +			domain |= RADEON_GEM_DOMAIN_GTT;
> +	}

The whole handling of placement.lpfn looks rather dubious... if I'm
reading it right, we're usually applying the VRAM size as a limit for
GTT as well? I guess your change doesn't make it any worse, but we
should probably clean this up at some point.

> @@ -102,6 +108,9 @@ int radeon_bo_create(struct radeon_device *rdev, struct drm_gem_object *gobj,
>  		type = ttm_bo_type_device;
>  	}
>  	*bo_ptr = NULL;
> +	if (size >= rdev->mc.active_vram_size && size >= rdev->mc.gtt_size) {
> +		return -ENOMEM;
> +	}

Couldn't it still work in the CPU domain? Though I guess a BO that can
never be moved into GTT or VRAM isn't very useful...

However, if the size exceeds GTT, I think this needs to compare against
visible_vram_size instead of active_vram_size, or it may not be possible
to map the BO?

-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel