[Mesa-dev] [PATCH] i965: Fix GLX_MESA_query_renderer video memory on 32-bit.

[Kenneth Graunke]

On Thursday, March 30, 2017 6:48:38 PM PDT Kenneth Graunke wrote:
> On Thursday, March 30, 2017 4:38:14 PM PDT Chris Wilson wrote:
> > On Thu, Mar 30, 2017 at 04:28:19PM -0700, Kenneth Graunke wrote:
> > > On modern systems with 4GB apertures, the size in bytes is 4294967296,
> > > or (1ull << 32).  The kernel gives us the aperture size as a __u64,
> > > which works out great.
> > > 
> > > Unfortunately, libdrm "helpfully" returns the data as a size_t, which
> > > on 32-bit systems means it truncates the aperture size to 0 bytes.
> > > We've happily reported this value as 0 MB of video memory via
> > > GLX_MESA_query_renderer since it was originally exposed.
> > > 
> > > This patch bypasses libdrm and calls the ioctl ourselves so we can
> > > use a proper uint64_t, avoiding the 32-bit integer overflow.  We now
> > > report a proper video memory size on 32-bit systems.
> > > ---
> > >  src/mesa/drivers/dri/i965/intel_screen.c | 16 ++++++++++++----
> > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> > > index 811a9c5a867..f94e8a77c10 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_screen.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> > > @@ -950,6 +950,17 @@ static const __DRIimageExtension intelImageExtension = {
> > >      .createImageWithModifiers           = intel_create_image_with_modifiers,
> > >  };
> > >  
> > > +static uint64_t
> > > +get_aperture_size(int fd)
> > > +{
> > > +   struct drm_i915_gem_get_aperture aperture;
> > > +
> > > +   if (drmIoctl(fd, DRM_IOCTL_I915_GEM_GET_APERTURE, &aperture) != 0)
> > > +      return 0;
> > 
> > The aperture is nothing to do with the video memory limits... You want
> > to query the context for the size of the GTT, e.g.
> > https://patchwork.freedesktop.org/patch/62189/
> > 
> > i.e.
> > static uint64_t get_gtt_size(int fd)
> > {
> >    struct drm_i915_gem_context_param p;
> >    size_t mappable_size, aper_size;
> > 
> >    memset(&p, 0, sizeof(p));
> >    p.param = I915_CONTEXT_PARAM_GTT_SIZE;
> >    if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, &p) == 0)
> >       return p.value;
> > 
> >    /* do sometheing useful for old kernels */
> > 
> >    drm_intel_get_aperture_sizes(fd, &mappable_size, &aper_size);
> > 
> >    return aper_size;
> > }
> 
> It's somewhat debatable what a unified memory GPU should return for a
> "Number of megabytes of video memory available to the renderer" query,
> as there really isn't a concept of video RAM.
> 
> When Ian implemented this, he chose to pick the amount of memory that
> a single batch can reference, which is 3/4 of the aperture.  This may
> be too small - applications can certainly use more memory than that.
> However, their working set for a draw had better fit within this limit,
> or else there will be a performance penalty.  I think it's pretty
> reasonable for what applications want.  They're trying to gauge how
> high-res their textures can be without incurring penalties.
> 
> I know Ian also spoke with a number of game vendors when drafting
> and implementing this extension, so I'm inclined to trust his
> interpretation.
> 
> I don't think exposing GTT_SIZE is useful.  With 48-bit addressing
> and PPGTT, the result of that query will be (1ull << 48) aka 256
> terabytes.  There is no way in hell that an application can use
> that much RAM.  We could restrict it to the total system RAM, but
> even then, it would not be performant to try and use all of RAM.

Okay, I missed that right below, we limit it to the amount of system
RAM.  So it'll never be 256 terabytes.  That's more reasonable.

Still not sure whether it's the right thing to do, though.

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170330/03e9a666/attachment-0001.sig>