[Beignet] [PATCH] driver/runtime: get global mem size dynamically

[Zhigang Gong]

You are right, you code is equivalent to my suggestion.
But I think it may be too conservative now. For an example,
if the system has 8GB memory, the max_mem_alloc_size will be
less or equal to 2GB. Maybe we can just set max_mem_alloc_size
equal to global_mem_size.

What do you think?

Even so, please be aware if the test machine has only 4GB
memory, it will never hit the GPU memory space above 2GB.

Thanks,
Zhigang Gong.

On Fri, Oct 09, 2015 at 08:56:38AM +0000, Pan, Xiuli wrote:
> So I have rewrite the code here, and I think we should not using totalgpumem for max_mem_alloc_size, the ret->global_mem_size / 2 will always less or equal to totalgpumem.
> Or I may not get your point about the threshold.
> 
> ret->global_mem_size = min(totalram/2 , totalgpumem) 
> ret->max_mem_alloc_size =  ret->global_mem_size / 2;
> 
> -----Original Message-----
> From: Zhigang Gong [mailto:zhigang.gong at linux.intel.com] 
> Sent: Friday, October 9, 2015 3:05 PM
> To: Pan, Xiuli <xiuli.pan at intel.com>
> Cc: beignet at lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH] driver/runtime: get global mem size dynamically
> 
> On Fri, Oct 09, 2015 at 03:56:11PM +0800, Pan Xiuli wrote:
> > The gen8 and higher gpu can have more than 2G mem, so we dynamically 
> > get the global mem size.
> > 
> > Signed-off-by: Pan Xiuli <xiuli.pan at intel.com>
> > ---
> >  src/cl_device_id.c       | 6 +++---
> >  src/intel/intel_driver.c | 6 ++++++
> >  2 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/cl_device_id.c b/src/cl_device_id.c index 
> > 78d2cf4..c3bd35f 100644
> > --- a/src/cl_device_id.c
> > +++ b/src/cl_device_id.c
> > @@ -550,10 +550,10 @@ skl_gt4_break:
> >  
> >    struct sysinfo info;
> >    if (sysinfo(&info) == 0) {
> > -    uint64_t two_gb = 2 * 1024 * 1024 * 1024ul; 
> > +    uint64_t totalgpumem = ret->global_mem_size;
> >      uint64_t totalram = info.totalram * info.mem_unit;
> > -    ret->global_mem_size = (totalram > two_gb) ? 
> > -                            two_gb : info.totalram;
> > +    ret->global_mem_size = (totalram > totalgpumem) ?
> > +                            totalgpumem: totalram;
> >      ret->max_mem_alloc_size = ret->global_mem_size / 2;
> 
> The "/ 2" is a hard coded method to avoid allocate too much memory which is not supported by the platform. Now this patch changes to get aperture size from libdrm, then we don't need to use it any more.
> 
> But we may still want to make sure that we don't allocate too much system memory. I think totalram/2 should be a reasonable threshold value.
> 
> So the following code may be better:
> 
> reg->global_mem_size = min(totalram/2, totalgpumem); max_mem_alloc_size 
> reg->= min(global_mem_size/2, totalgpumem);
> 
> Furthermore we still need to check carefully whether it's safe to use totalgpumem as the global_mem_size and/or max_mem_alloc_size.
> 
> If the test case tests the maximum memory size, and there are some aperture space already allocated by system, then the case may be failed. Before submit the patch, it's better to test it with all the related conformance test cases on all platforms. Or maybe we need to add some cases to test edge conditions into the internal unit test cases.
> 
> Thanks,
> Zhigang Gong.
> 
> >    }
> >  
> > diff --git a/src/intel/intel_driver.c b/src/intel/intel_driver.c index 
> > 035a103..1f286f7 100644
> > --- a/src/intel/intel_driver.c
> > +++ b/src/intel/intel_driver.c
> > @@ -829,6 +829,12 @@ intel_update_device_info(cl_device_id device)
> >    if (IS_CHERRYVIEW(device->device_id))
> >      printf(CHV_CONFIG_WARNING);
> >  #endif
> > +  //We should get the device memory dynamically, also the  //mapablce 
> > + mem size usage is unknown. Still use global_mem_size/2  //as 
> > + max_mem_alloc_size in cl_get_gt_device.
> > +  size_t total_mem,map_mem;
> > +  drm_intel_get_aperture_sizes(driver->fd,&map_mem,&total_mem);
> > +  device->global_mem_size = (cl_ulong)total_mem;
> >  
> >    intel_driver_context_destroy(driver);
> >    intel_driver_close(driver);
> > --
> > 2.1.4
> > 
> > _______________________________________________
> > Beignet mailing list
> > Beignet at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet