[Intel-gfx] [PATCH] drm/i915: Handle request to map a very large buffer object

[Paul Menzel]

Dear Anuj,


thank you for your patch with the great explanation in the commit
message. I found one typo in the code comment and while at it point out
some grammar issue in the commit message.


Am Montag, den 27.02.2012, 12:28 -0800 schrieb Anuj Phogat:
> This patch sets an upper limit on the size of buffer object which can be
> mapped safely. Following bugs reported segmentation fault / assertion

reported *a* segmentation fault / *an* assertion …

> failure with large textures:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=44970
> https://bugs.freedesktop.org/show_bug.cgi?id=46303
> 
> This patch along with another patch which I posted on mesa-dev
> (intel: Fix a case when mapping large texture fails) [1] resolve

You could add the URL.

> above mentioned bugs. Recently posted piglit test case (large-textures)

*The* recently …

> also passes with these patches.

[1] http://lists.freedesktop.org/archives/mesa-dev/2012-February/019488.html

> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> ---
> This fix doesn't limit developers to create very large texture but it
> restricts them to map it. I am not very confident about this patch and
> setting 128 MB as the upper limit on size of buffer object. So, please
> provide your views.
> 
> Chris Wilson's views on this issue:
> http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg08585.html
> 
>  intel/intel_bufmgr_gem.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 0f33b71..8b05de9 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -1191,6 +1191,17 @@ int drm_intel_gem_bo_map_gtt(drm_intel_bo *bo)
>  
>  	pthread_mutex_lock(&bufmgr_gem->lock);
>  
> +	/* Set am upper limit on the size of buffer which can be mapped
> +	   safely

s/am/an/
Maybe add full stop ».« at the end of the sentence.

> +	 */
> +	if (bo->size > 128 * 1024 * 1024) {

It seems strange for me that the upper buffer size is always the same
and not dependent on the chipset(?). But I guess you have checked that.

> +		DBG("%s:%d: Reached buffer map limit.\n",
> +		    __FILE__, __LINE__);
> +		bo->virtual = NULL;
> +		pthread_mutex_unlock(&bufmgr_gem->lock);
> +		return -1;
> +	}
> +
>  	if (bo_gem->map_count++ == 0)
>  		drm_intel_gem_bo_open_vma(bufmgr_gem, bo_gem);

Reviewed-by: Paul Menzel <paulepanter at users.sourceforge.net>


Thanks,

Paul

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20120228/d04e6640/attachment.sig>