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.pho...@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 <paulepan...@users.sourceforge.net>


Thanks,

Paul

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to