subject:"Re\: \[Mesa\-dev\] \[PATCH 07\/18\] i915\: Wire up initial support for DRI_RENDERER

Re: [Mesa-dev] [PATCH 07/18] i915: Wire up initial support for DRI_RENDERER_QUERY extension

2013-11-11 Thread Ian Romanick

On 11/09/2013 02:44 AM, Daniel Vetter wrote:
 On Fri, Oct 11, 2013 at 03:10:14PM -0700, Ian Romanick wrote:
 From: Ian Romanick ian.d.roman...@intel.com

 Signed-off-by: Ian Romanick ian.d.roman...@intel.com
 ---
  src/mesa/drivers/dri/i915/intel_screen.c | 79 
 
  1 file changed, 79 insertions(+)

 diff --git a/src/mesa/drivers/dri/i915/intel_screen.c 
 b/src/mesa/drivers/dri/i915/intel_screen.c
 index 4f8c342..fa4fdc0 100644
 --- a/src/mesa/drivers/dri/i915/intel_screen.c
 +++ b/src/mesa/drivers/dri/i915/intel_screen.c
 @@ -27,6 +27,7 @@
  
  #include errno.h
  #include time.h
 +#include sys/sysinfo.h
  #include main/glheader.h
  #include main/context.h
  #include main/framebuffer.h
 @@ -741,6 +742,84 @@ static struct __DRIimageExtensionRec 
 intelImageExtension = {
  .createImageFromFds = intel_create_image_from_fds
  };
  
 +static int
 +i915_query_renderer_integer(__DRIscreen *psp, int param, int *value)
 +{
 +   const struct intel_screen *const intelScreen =
 +  (struct intel_screen *) psp-driverPrivate;
 +
 +   switch (param) {
 +   case __DRI2_RENDERER_VENDOR_ID:
 +  value[0] = 0x8086;
 +  return 0;
 +   case __DRI2_RENDERER_DEVICE_ID:
 +  value[0] = intelScreen-deviceID;
 +  return 0;
 +   case __DRI2_RENDERER_ACCELERATED:
 +  value[0] = 1;
 +  return 0;
 +   case __DRI2_RENDERER_VIDEO_MEMORY: {
 +  struct sysinfo info;
 +  uint64_t system_memory_bytes;
 +  unsigned system_memory_megabytes;
 +
 +  /* Once a batch uses more than 75% of the maximum mappable size, we
 +   * assume that there's some fragmentation, and we start doing extra
 +   * flushing, etc.  That's the big cliff apps will care about.
 +   */
 +  const unsigned long agp_bytes = drmAgpSize(psp-fd);
 
 So despite me shooting at this in the next patch saying that this is
 - the wrong interface, it doesn't actually really tell you what you want
   to know (since it fails to take pinnned crap into account),
 - doesn't work on half the platforms i915_dri supports already,
 - and is massively deprecated on all others and a major pain for us to
   keep on live support in the kernel

In fairness, you missed this specific issue on your first review and
shot at it after I committed it. :(  There was no malice... just
timezone fail.  In the future, I'll CC you any Mesa changes that
interact with the kernel so that you'll notice them sooner.

 you've decided to raise this particular zombie and commited it shortly
 before the branch point. Please rip this out asap before it shows up
 anywhere in a release and use the gem aperture ioctl instead.
 
 That would also fix things for gen4, where the hardwired 2G isn't really
 the truth of things either.

 Yours, decently pissed,
 -Daniel

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 07/18] i915: Wire up initial support for DRI_RENDERER_QUERY extension

2013-11-11 Thread Daniel Vetter

On Mon, Nov 11, 2013 at 11:03:49AM -0800, Ian Romanick wrote:
 On 11/09/2013 02:44 AM, Daniel Vetter wrote:
  On Fri, Oct 11, 2013 at 03:10:14PM -0700, Ian Romanick wrote:
  From: Ian Romanick ian.d.roman...@intel.com
 
  Signed-off-by: Ian Romanick ian.d.roman...@intel.com
  ---
   src/mesa/drivers/dri/i915/intel_screen.c | 79 
  
   1 file changed, 79 insertions(+)
 
  diff --git a/src/mesa/drivers/dri/i915/intel_screen.c 
  b/src/mesa/drivers/dri/i915/intel_screen.c
  index 4f8c342..fa4fdc0 100644
  --- a/src/mesa/drivers/dri/i915/intel_screen.c
  +++ b/src/mesa/drivers/dri/i915/intel_screen.c
  @@ -27,6 +27,7 @@
   
   #include errno.h
   #include time.h
  +#include sys/sysinfo.h
   #include main/glheader.h
   #include main/context.h
   #include main/framebuffer.h
  @@ -741,6 +742,84 @@ static struct __DRIimageExtensionRec 
  intelImageExtension = {
   .createImageFromFds = intel_create_image_from_fds
   };
   
  +static int
  +i915_query_renderer_integer(__DRIscreen *psp, int param, int *value)
  +{
  +   const struct intel_screen *const intelScreen =
  +  (struct intel_screen *) psp-driverPrivate;
  +
  +   switch (param) {
  +   case __DRI2_RENDERER_VENDOR_ID:
  +  value[0] = 0x8086;
  +  return 0;
  +   case __DRI2_RENDERER_DEVICE_ID:
  +  value[0] = intelScreen-deviceID;
  +  return 0;
  +   case __DRI2_RENDERER_ACCELERATED:
  +  value[0] = 1;
  +  return 0;
  +   case __DRI2_RENDERER_VIDEO_MEMORY: {
  +  struct sysinfo info;
  +  uint64_t system_memory_bytes;
  +  unsigned system_memory_megabytes;
  +
  +  /* Once a batch uses more than 75% of the maximum mappable size, we
  +   * assume that there's some fragmentation, and we start doing extra
  +   * flushing, etc.  That's the big cliff apps will care about.
  +   */
  +  const unsigned long agp_bytes = drmAgpSize(psp-fd);
  
  So despite me shooting at this in the next patch saying that this is
  - the wrong interface, it doesn't actually really tell you what you want
to know (since it fails to take pinnned crap into account),
  - doesn't work on half the platforms i915_dri supports already,
  - and is massively deprecated on all others and a major pain for us to
keep on live support in the kernel
 
 In fairness, you missed this specific issue on your first review and
 shot at it after I committed it. :(  There was no malice... just
 timezone fail.  In the future, I'll CC you any Mesa changes that
 interact with the kernel so that you'll notice them sooner.

Yeah, I try to read most of mesa-devel, but can't really go into details
of the patches everywhere. I only replied to the i965 patch since Ken's
review made me curious to check the details. Then your reply later on made
me check the previous patch.

Anyway, I've cooled off now, just happy that we've caught this zoombie in
the nick of time ;-)

Another thing I've noticed is that you adjust the advertised vram size
with the available memory. I don't think that's an issue already since the
aperture space checker in libdrm (which mesa uses to avoid overfilling
batches) doesn't do that. But it'll be one on memory constrained
phones/tablets and also with the much bigger gtt on bdw.

I think the right fix for that is to adjust the
aperture.aper_available_size in the kernel, so that all users of this
interface have correct data. The kernel already adjusts this for pinned
objects and similar stuff, so would fit neatly. I'll wip up a kernel patch
for that. I guess we could leave the current stuff in for 10.0 and remove
it in master once the kernel fix has landed.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 07/18] i915: Wire up initial support for DRI_RENDERER_QUERY extension

2013-11-09 Thread Daniel Vetter

On Fri, Oct 11, 2013 at 03:10:14PM -0700, Ian Romanick wrote:
 From: Ian Romanick ian.d.roman...@intel.com
 
 Signed-off-by: Ian Romanick ian.d.roman...@intel.com
 ---
  src/mesa/drivers/dri/i915/intel_screen.c | 79 
 
  1 file changed, 79 insertions(+)
 
 diff --git a/src/mesa/drivers/dri/i915/intel_screen.c 
 b/src/mesa/drivers/dri/i915/intel_screen.c
 index 4f8c342..fa4fdc0 100644
 --- a/src/mesa/drivers/dri/i915/intel_screen.c
 +++ b/src/mesa/drivers/dri/i915/intel_screen.c
 @@ -27,6 +27,7 @@
  
  #include errno.h
  #include time.h
 +#include sys/sysinfo.h
  #include main/glheader.h
  #include main/context.h
  #include main/framebuffer.h
 @@ -741,6 +742,84 @@ static struct __DRIimageExtensionRec intelImageExtension 
 = {
  .createImageFromFds = intel_create_image_from_fds
  };
  
 +static int
 +i915_query_renderer_integer(__DRIscreen *psp, int param, int *value)
 +{
 +   const struct intel_screen *const intelScreen =
 +  (struct intel_screen *) psp-driverPrivate;
 +
 +   switch (param) {
 +   case __DRI2_RENDERER_VENDOR_ID:
 +  value[0] = 0x8086;
 +  return 0;
 +   case __DRI2_RENDERER_DEVICE_ID:
 +  value[0] = intelScreen-deviceID;
 +  return 0;
 +   case __DRI2_RENDERER_ACCELERATED:
 +  value[0] = 1;
 +  return 0;
 +   case __DRI2_RENDERER_VIDEO_MEMORY: {
 +  struct sysinfo info;
 +  uint64_t system_memory_bytes;
 +  unsigned system_memory_megabytes;
 +
 +  /* Once a batch uses more than 75% of the maximum mappable size, we
 +   * assume that there's some fragmentation, and we start doing extra
 +   * flushing, etc.  That's the big cliff apps will care about.
 +   */
 +  const unsigned long agp_bytes = drmAgpSize(psp-fd);

So despite me shooting at this in the next patch saying that this is
- the wrong interface, it doesn't actually really tell you what you want
  to know (since it fails to take pinnned crap into account),
- doesn't work on half the platforms i915_dri supports already,
- and is massively deprecated on all others and a major pain for us to
  keep on live support in the kernel

you've decided to raise this particular zombie and commited it shortly
before the branch point. Please rip this out asap before it shows up
anywhere in a release and use the gem aperture ioctl instead.

That would also fix things for gen4, where the hardwired 2G isn't really
the truth of things either.

Yours, decently pissed,
-Daniel

 +  const unsigned gpu_mappable_megabytes =
 + (agp_bytes / (1024 * 1024)) * 3 / 4;
 +
 +  if (sysinfo(info)  0)
 + return -1;
 +
 +  system_memory_bytes = ((uint64_t) info.totalram) * info.mem_unit;
 +  system_memory_megabytes = (unsigned) (system_memory_bytes / 1024);
 +
 +  value[0] = MIN2(system_memory_megabytes, gpu_mappable_megabytes);
 +  return 0;
 +   }
 +   case __DRI2_RENDERER_UNIFIED_MEMORY_ARCHITECTURE:
 +  value[0] = 1;
 +  return 0;
 +   case __DRI2_RENDERER_PREFERRED_PROFILE:
 +  value[0] = (1U  __DRI_API_OPENGL);
 +  return 0;
 +   default:
 +  return driQueryRendererIntegerCommon(psp, param, value);
 +   }
 +
 +   return -1;
 +}
 +
 +static int
 +i915_query_renderer_string(__DRIscreen *psp, int param, const char **value)
 +{
 +   const struct intel_screen *intelScreen =
 +  (struct intel_screen *) psp-driverPrivate;
 +
 +   switch (param) {
 +   case __DRI2_RENDERER_VENDOR_ID:
 +  value[0] = i915_vendor_string;
 +  return 0;
 +   case __DRI2_RENDERER_DEVICE_ID:
 +  value[0] = i915_get_renderer_string(intelScreen-deviceID);
 +  return 0;
 +   default:
 +  break;
 +   }
 +
 +   return -1;
 +}
 +
 +static struct __DRI2rendererQueryExtensionRec intelRendererQueryExtension = {
 +   .base = { __DRI2_RENDERER_QUERY, 1 },
 +
 +   .queryInteger = i915_query_renderer_integer,
 +   .queryString = i915_query_renderer_string
 +};
 +
  static const __DRIextension *intelScreenExtensions[] = {
  intelTexBufferExtension.base,
  intelFlushExtension.base,
 -- 
 1.8.1.4
 
 ___
 mesa-dev mailing list
 mesa-dev@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/mesa-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 07/18] i915: Wire up initial support for DRI_RENDERER_QUERY extension

Re: [Mesa-dev] [PATCH 07/18] i915: Wire up initial support for DRI_RENDERER_QUERY extension

Re: [Mesa-dev] [PATCH 07/18] i915: Wire up initial support for DRI_RENDERER_QUERY extension

3 matches

Site Navigation

Mail list logo

Footer information