Re: [Intel-gfx] [Mesa-dev] [PATCH v3 3/4] drm/i915/uapi: convert i915_query and friend to kernel doc

[Ian Romanick]

On 4/15/21 8:59 AM, Matthew Auld wrote:
> Add a note about the two-step process.
> 
> Suggested-by: Daniel Vetter 
> Signed-off-by: Matthew Auld 
> Cc: Joonas Lahtinen 
> Cc: Jordan Justen 
> Cc: Daniel Vetter 
> Cc: Kenneth Graunke 
> Cc: Jason Ekstrand 
> Cc: Dave Airlie 
> Cc: dri-de...@lists.freedesktop.org
> Cc: mesa-...@lists.freedesktop.org
> ---
>  include/uapi/drm/i915_drm.h | 57 ++---
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index d9c954a5a456..ef36f1a0adde 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -2210,14 +2210,23 @@ struct drm_i915_perf_oa_config {
>   __u64 flex_regs_ptr;
>  };
>  
> +/**
> + * struct drm_i915_query_item - An individual query for the kernel to 
> process.
> + *
> + * The behaviour is determined by the @query_id. Note that exactly what

Since we just had a big discussion about this on mesa-dev w.r.t. Mesa
code and documentation... does the kernel have a policy about which
flavor (pun intended) of English should be used?

> + * @data_ptr is also depends on the specific @query_id.
> + */
>  struct drm_i915_query_item {
> + /** @query_id: The id for this query */
>   __u64 query_id;
>  #define DRM_I915_QUERY_TOPOLOGY_INFO1
>  #define DRM_I915_QUERY_ENGINE_INFO   2
>  #define DRM_I915_QUERY_PERF_CONFIG  3
>  /* Must be kept compact -- no holes and well documented */
>  
> - /*
> + /**
> +  * @length:
> +  *
>* When set to zero by userspace, this is filled with the size of the
>* data to be written at the data_ptr pointer. The kernel sets this
>* value to a negative value to signal an error on a particular query
> @@ -2225,21 +2234,26 @@ struct drm_i915_query_item {
>*/
>   __s32 length;
>  
> - /*
> + /**
> +  * @flags:
> +  *
>* When query_id == DRM_I915_QUERY_TOPOLOGY_INFO, must be 0.
>*
>* When query_id == DRM_I915_QUERY_PERF_CONFIG, must be one of the
> -  * following :
> -  * - DRM_I915_QUERY_PERF_CONFIG_LIST
> -  * - DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_UUID
> -  * - DRM_I915_QUERY_PERF_CONFIG_FOR_UUID
> +  * following:
> +  *
> +  *  - DRM_I915_QUERY_PERF_CONFIG_LIST
> +  *  - DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_UUID
> +  *  - DRM_I915_QUERY_PERF_CONFIG_FOR_UUID
>*/
>   __u32 flags;
>  #define DRM_I915_QUERY_PERF_CONFIG_LIST  1
>  #define DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_UUID 2
>  #define DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_ID   3
>  
> - /*
> + /**
> +  * @data_ptr:
> +  *
>* Data will be written at the location pointed by data_ptr when the
>* value of length matches the length of the data to be written by the
>* kernel.
> @@ -2247,16 +2261,37 @@ struct drm_i915_query_item {
>   __u64 data_ptr;
>  };
>  
> +/**
> + * struct drm_i915_query - Supply an array of drm_i915_query_item for the 
> kernel
> + * to fill out.
> + *
> + * Note that this is generally a two step process for each 
> drm_i915_query_item
> + * in the array:
> + *
> + *   1.) Call the DRM_IOCTL_I915_QUERY, giving it our array of
> + *   drm_i915_query_item, with drm_i915_query_item.size set to zero. The
> + *   kernel will then fill in the size, in bytes, which tells userspace how
> + *   memory it needs to allocate for the blob(say for an array of
> + *   properties).
> + *
> + *   2.) Next we call DRM_IOCTL_I915_QUERY again, this time with the
> + *   drm_i915_query_item.data_ptr equal to our newly allocated blob. Note
> + *   that the i915_query_item.size should still be the same as what the
> + *   kernel previously set. At this point the kernel can fill in the blob.
> + *
> + */
>  struct drm_i915_query {
> + /** @num_items: The number of elements in the @items_ptr array */
>   __u32 num_items;
>  
> - /*
> -  * Unused for now. Must be cleared to zero.
> + /**
> +  * @flags: Unused for now. Must be cleared to zero.
>*/
>   __u32 flags;
>  
> - /*
> -  * This points to an array of num_items drm_i915_query_item structures.
> + /**
> +  * @items_ptr: This points to an array of num_items drm_i915_query_item
> +  * structures.
>*/
>   __u64 items_ptr;
>  };
> 

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Mesa-dev] [PATCH] i965: Embrace "unlimited" GTT mmap support

[Ian Romanick]

On 08/24/2016 12:42 PM, Chris Wilson wrote:
> From about kernel 4.9, GTT mmaps are virtually unlimited. A new
> parameter, I915_PARAM_MMAP_GTT_VERSION, is added to advertise the
> feature so query it and use it to avoid limiting tiled allocations to
> only fit within the mappable aperture.
> 
> Signed-off-by: Chris Wilson 
> Cc: Kenneth Graunke 
> ---
>  src/mesa/drivers/dri/i965/brw_context.c  | 17 ++---
>  src/mesa/drivers/dri/i965/brw_context.h  |  2 +-
>  src/mesa/drivers/dri/i965/intel_screen.c | 32 
> 
>  src/mesa/drivers/dri/i965/intel_screen.h |  2 ++
>  4 files changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
> b/src/mesa/drivers/dri/i965/brw_context.c
> index b0f9063..79dba1e 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -1033,21 +1033,6 @@ brwCreateContext(gl_api api,
> brw->urb.max_ds_entries = devinfo->urb.max_ds_entries;
> brw->urb.max_gs_entries = devinfo->urb.max_gs_entries;
>  
> -   /* Estimate the size of the mappable aperture into the GTT.  There's an
> -* ioctl to get the whole GTT size, but not one to get the mappable 
> subset.
> -* It turns out it's basically always 256MB, though some ancient hardware
> -* was smaller.
> -*/
> -   uint32_t gtt_size = 256 * 1024 * 1024;
> -
> -   /* We don't want to map two objects such that a memcpy between them would
> -* just fault one mapping in and then the other over and over forever.  So
> -* we would need to divide the GTT size by 2.  Additionally, some GTT is
> -* taken up by things like the framebuffer and the ringbuffer and such, so
> -* be more conservative.
> -*/
> -   brw->max_gtt_map_object_size = gtt_size / 4;
> -
> if (brw->gen == 6)
>brw->urb.gs_present = false;
>  
> @@ -1058,6 +1043,8 @@ brwCreateContext(gl_api api,
>  
> brw->predicate.state = BRW_PREDICATE_STATE_RENDER;
>  
> +   brw->max_gtt_map_object_size = screen->max_gtt_map_object_size;
> +
> brw->use_resource_streamer = screen->has_resource_streamer &&
>(env_var_as_boolean("INTEL_USE_HW_BT", false) ||
> env_var_as_boolean("INTEL_USE_GATHER", false));
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
> b/src/mesa/drivers/dri/i965/brw_context.h
> index f2dd164..523f36c 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -861,7 +861,7 @@ struct brw_context
>  */
> bool perf_debug;
>  
> -   uint32_t max_gtt_map_object_size;
> +   uint64_t max_gtt_map_object_size;
>  
> int gen;
> int gt;
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c 
> b/src/mesa/drivers/dri/i965/intel_screen.c
> index 98f1c76..62eacba 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -1006,6 +1006,17 @@ intel_get_boolean(struct intel_screen *screen, int 
> param)
> return (intel_get_param(screen, param, &value) == 0) && value;
>  }
>  
> +static int
> +intel_get_integer(struct intel_screen *screen, int param)
> +{
> +   int value = -1;
> +
> +   if (intel_get_param(screen, param, &value) == 0)
> +return value;
> +
> +   return -1;
> +}
> +
>  static void
>  intelDestroyScreen(__DRIscreen * sPriv)
>  {
> @@ -1576,6 +1587,27 @@ __DRIconfig **intelInitScreen2(__DRIscreen *psp)
> if (INTEL_DEBUG & DEBUG_AUB)
>drm_intel_bufmgr_gem_set_aub_dump(intelScreen->bufmgr, true);
>  
> +#define I915_PARAM_MMAP_GTT_VERSION 40 /* XXX delete me with new libdrm */
> +   if (intel_get_integer(intelScreen, I915_PARAM_MMAP_GTT_VERSION) >= 1) {
> +  /* Theorectically unlimited! */
> +  intelScreen->max_gtt_map_object_size = UINT64_MAX;

Well... not quite unlimited, right?  Isn't the actual VMA space less
than 64-bits?  I thought it was more like 48 bits.

> +   } else {
> +  /* Estimate the size of the mappable aperture into the GTT.  There's an
> +   * ioctl to get the whole GTT size, but not one to get the mappable 
> subset.
> +   * It turns out it's basically always 256MB, though some ancient 
> hardware
> +   * was smaller.
> +   */
> +  uint32_t gtt_size = 256 * 1024 * 1024;
> +
> +  /* We don't want to map two objects such that a memcpy between them 
> would
> +   * just fault one mapping in and then the other over and over forever. 
>  So
> +   * we would need to divide the GTT size by 2.  Additionally, some GTT 
> is
> +   * taken up by things like the framebuffer and the ringbuffer and 
> such, so
> +   * be more conservative.
> +   */
> +  intelScreen->max_gtt_map_object_size = gtt_size / 4;
> +   }
> +
> intelScreen->hw_has_swizzling = intel_detect_swizzling(intelScreen);
> intelScreen->hw_has_timestamp = intel_detect_timestamp(intelScreen);
>  
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.h 
> b/src/mesa/drivers/dri/i965/intel_screen.h
>

Re: [Intel-gfx] [PATCH libdrm 00/12] Introduce drm_intel_device and use i915_pciid.h

[Ian Romanick]

Based on light reading, patches 1, 5, 6, 7, 8, 10, and 11 are

Reviewed-by: Ian Romanick 

I sent a comment on patch 9.  I'll try to look at the others in the next
few days... assuming nobody beats me to it.

I'm also going to send some similar Mesa patches that I'll CC you on.

On 03/05/2015 08:20 AM, Damien Lespiau wrote:
> A couple of things I wanted to do for the longest time:
>   
>   - Have (intel's) libdrm use the kernel i915_pciids.h so we can just copy the
> file when updating
>   - Start a new object, struct drm_intel_device where we could put common code
> across several userspace projects. For instance it could be where we put
> the "number of threads" logic we need to use in several 3d/gpgpu
> states/instructions (that's a bit fiddly starting with CHV: we can't use
> static tables anymore and need a runtime query to the kernel)
> 
> I tested it a bit so it can't be totally wrong:
> 
>   - I ran with this series on a couple of machines with no noticeable problem
>   - I check that the INTEL_DEVID_OVERRIDE env variable was still working (to
> dump AUB files)
>   - make check, which exercises changes in the decoder path, still passes
> 

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

Re: [Intel-gfx] [PATCH 09/12] intel: Provide IS_GENX() macros taking a drm_intel_device as argument

[Ian Romanick]

On 03/05/2015 08:20 AM, Damien Lespiau wrote:
> Time to switch over all the IS_GENX() macros to the new device object.
> Nothing more than a mechanical search & replace here.

Hmm... why not just do the comparisons directly?  The macros seem
superfluous.

> Signed-off-by: Damien Lespiau 
> ---
>  intel/intel_bufmgr_gem.c  |   7 +-
>  intel/intel_chipset.h | 158 
> --
>  intel/intel_decode.c  |  41 ++--
>  intel/intel_device_priv.h |   8 +++
>  4 files changed, 31 insertions(+), 183 deletions(-)
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 58543a2..011fa5b 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -3451,8 +3451,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
>   bufmgr_gem->pci_device = drm_intel_device_get_devid(bufmgr_gem->dev);
>   bufmgr_gem->gen = bufmgr_gem->dev->gen;
>  
> - if (IS_GEN3(bufmgr_gem->pci_device) &&
> - bufmgr_gem->gtt_size > 256*1024*1024) {
> + if (IS_GEN3(bufmgr_gem->dev) && bufmgr_gem->gtt_size > 256*1024*1024) {
>   /* The unmappable part of gtt on gen 3 (i.e. above 256MB) can't
>* be used for tiled blits. To simplify the accounting, just
>* substract the unmappable part (fixed to 256MB on all known
> @@ -3494,8 +3493,8 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
>   /* Kernel does not supports HAS_LLC query, fallback to GPU
>* generation detection and assume that we have LLC on GEN6/7
>*/
> - bufmgr_gem->has_llc = IS_GEN6(bufmgr_gem->pci_device) ||
> -   IS_GEN7(bufmgr_gem->pci_device);
> + bufmgr_gem->has_llc = IS_GEN6(bufmgr_gem->dev) ||
> +   IS_GEN7(bufmgr_gem->dev);
>   } else
>   bufmgr_gem->has_llc = *gp.value;
>  
> diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h
> index 241d700..134c877 100644
> --- a/intel/intel_chipset.h
> +++ b/intel/intel_chipset.h
> @@ -181,162 +181,4 @@
>  #define PCI_CHIP_SKYLAKE_SRV_GT1 0x190A
>  #define PCI_CHIP_SKYLAKE_WKS_GT2 0x191D
>  
> -#define IS_ILD(devid)((devid) == PCI_CHIP_ILD_G)
> -#define IS_ILM(devid)((devid) == PCI_CHIP_ILM_G)
> -
> -#define IS_915(devid)((devid) == PCI_CHIP_I915_G || \
> -  (devid) == PCI_CHIP_E7221_G || \
> -  (devid) == PCI_CHIP_I915_GM)
> -
> -#define IS_945GM(devid)  ((devid) == PCI_CHIP_I945_GM || \
> -  (devid) == PCI_CHIP_I945_GME)
> -
> -#define IS_945(devid)((devid) == PCI_CHIP_I945_G || \
> -  (devid) == PCI_CHIP_I945_GM || \
> -  (devid) == PCI_CHIP_I945_GME || \
> -  IS_G33(devid))
> -
> -#define IS_G33(devid)((devid) == PCI_CHIP_G33_G || \
> -  (devid) == PCI_CHIP_Q33_G || \
> -  (devid) == PCI_CHIP_Q35_G || IS_IGD(devid))
> -
> -#define IS_GEN2(devid)   ((devid) == PCI_CHIP_I830_M || \
> -  (devid) == PCI_CHIP_845_G || \
> -  (devid) == PCI_CHIP_I855_GM || \
> -  (devid) == PCI_CHIP_I865_G)
> -
> -#define IS_GEN3(devid)   (IS_945(devid) || IS_915(devid))
> -
> -#define IS_GEN5(devid)   (IS_ILD(devid) || IS_ILM(devid))
> -
> -#define IS_GEN6(devid)   ((devid) == PCI_CHIP_SANDYBRIDGE_GT1 || 
> \
> -  (devid) == PCI_CHIP_SANDYBRIDGE_GT2 || \
> -  (devid) == PCI_CHIP_SANDYBRIDGE_GT2_PLUS || \
> -  (devid) == PCI_CHIP_SANDYBRIDGE_M_GT1 || \
> -  (devid) == PCI_CHIP_SANDYBRIDGE_M_GT2 || \
> -  (devid) == PCI_CHIP_SANDYBRIDGE_M_GT2_PLUS || \
> -  (devid) == PCI_CHIP_SANDYBRIDGE_S)
> -
> -#define IS_GEN7(devid)   (IS_IVYBRIDGE(devid) || \
> -  IS_HASWELL(devid) || \
> -  IS_VALLEYVIEW(devid))
> -
> -#define IS_IVYBRIDGE(devid)  ((devid) == PCI_CHIP_IVYBRIDGE_GT1 || \
> -  (devid) == PCI_CHIP_IVYBRIDGE_GT2 || \
> -  (devid) == PCI_CHIP_IVYBRIDGE_M_GT1 || \
> -  (devid) == PCI_CHIP_IVYBRIDGE_M_GT2 || \
> -  (devid) == PCI_CHIP_IVYBRIDGE_S || \
> -  (devid) == PCI_CHIP_IVYBRIDGE_S_GT2)
> -
> -#define IS_VALLEYVIEW(devid) ((devid) == PCI_CHIP_VALLEYVIEW_PO || \
> -  (devid) == PCI_CHIP_VALLEYVIEW_1 || \
> -  (devid) == PCI_CHIP_VALLEYVIEW_2 || \
> - 

Re: [Intel-gfx] [PATCH 2/4] xf86drmMode: Unconditionally clear ioctl structs

[Ian Romanick]

This patch is

Reviewed-by: Ian Romanick 

On 02/11/2015 03:42 AM, Daniel Vetter wrote:
> We really have to do this to avoid surprises when extending the ABI
> later on. Especially when growing the structures.
> 
> Signed-off-by: Daniel Vetter 
> ---
>  xf86drmMode.c | 55 ---
>  1 file changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/xf86drmMode.c b/xf86drmMode.c
> index e3e599bdc39d..9ea8fe721842 100644
> --- a/xf86drmMode.c
> +++ b/xf86drmMode.c
> @@ -61,7 +61,7 @@
>  #define VG(x)
>  #endif
>  
> -#define VG_CLEAR(s) VG(memset(&s, 0, sizeof(s)))
> +#define memclear(s) memset(&s, 0, sizeof(s))
>  
>  #define U642VOID(x) ((void *)(unsigned long)(x))
>  #define VOID2U64(x) ((uint64_t)(unsigned long)(x))
> @@ -164,7 +164,7 @@ drmModeResPtr drmModeGetResources(int fd)
>   drmModeResPtr r = 0;
>  
>  retry:
> - memset(&res, 0, sizeof(struct drm_mode_card_res));
> + memclear(res);
>   if (drmIoctl(fd, DRM_IOCTL_MODE_GETRESOURCES, &res))
>   return 0;
>  
> @@ -259,7 +259,7 @@ int drmModeAddFB(int fd, uint32_t width, uint32_t height, 
> uint8_t depth,
>   struct drm_mode_fb_cmd f;
>   int ret;
>  
> - VG_CLEAR(f);
> + memclear(f);
>   f.width  = width;
>   f.height = height;
>   f.pitch  = pitch;
> @@ -282,6 +282,7 @@ int drmModeAddFB2(int fd, uint32_t width, uint32_t height,
>   struct drm_mode_fb_cmd2 f;
>   int ret;
>  
> + memclear(f);
>   f.width  = width;
>   f.height = height;
>   f.pixel_format = pixel_format;
> @@ -300,8 +301,6 @@ int drmModeAddFB2(int fd, uint32_t width, uint32_t height,
>  int drmModeRmFB(int fd, uint32_t bufferId)
>  {
>   return DRM_IOCTL(fd, DRM_IOCTL_MODE_RMFB, &bufferId);
> -
> -
>  }
>  
>  drmModeFBPtr drmModeGetFB(int fd, uint32_t buf)
> @@ -309,6 +308,7 @@ drmModeFBPtr drmModeGetFB(int fd, uint32_t buf)
>   struct drm_mode_fb_cmd info;
>   drmModeFBPtr r;
>  
> + memclear(info);
>   info.fb_id = buf;
>  
>   if (drmIoctl(fd, DRM_IOCTL_MODE_GETFB, &info))
> @@ -331,8 +331,9 @@ drmModeFBPtr drmModeGetFB(int fd, uint32_t buf)
>  int drmModeDirtyFB(int fd, uint32_t bufferId,
>  drmModeClipPtr clips, uint32_t num_clips)
>  {
> - struct drm_mode_fb_dirty_cmd dirty = { 0 };
> + struct drm_mode_fb_dirty_cmd dirty;
>  
> + memclear(dirty);
>   dirty.fb_id = bufferId;
>   dirty.clips_ptr = VOID2U64(clips);
>   dirty.num_clips = num_clips;
> @@ -350,7 +351,7 @@ drmModeCrtcPtr drmModeGetCrtc(int fd, uint32_t crtcId)
>   struct drm_mode_crtc crtc;
>   drmModeCrtcPtr r;
>  
> - VG_CLEAR(crtc);
> + memclear(crtc);
>   crtc.crtc_id = crtcId;
>  
>   if (drmIoctl(fd, DRM_IOCTL_MODE_GETCRTC, &crtc))
> @@ -384,7 +385,7 @@ int drmModeSetCrtc(int fd, uint32_t crtcId, uint32_t 
> bufferId,
>  {
>   struct drm_mode_crtc crtc;
>  
> - VG_CLEAR(crtc);
> + memclear(crtc);
>   crtc.x = x;
>   crtc.y = y;
>   crtc.crtc_id   = crtcId;
> @@ -394,8 +395,7 @@ int drmModeSetCrtc(int fd, uint32_t crtcId, uint32_t 
> bufferId,
>   if (mode) {
> memcpy(&crtc.mode, mode, sizeof(struct drm_mode_modeinfo));
> crtc.mode_valid = 1;
> - } else
> -   crtc.mode_valid = 0;
> + }
>  
>   return DRM_IOCTL(fd, DRM_IOCTL_MODE_SETCRTC, &crtc);
>  }
> @@ -408,6 +408,7 @@ int drmModeSetCursor(int fd, uint32_t crtcId, uint32_t 
> bo_handle, uint32_t width
>  {
>   struct drm_mode_cursor arg;
>  
> + memclear(arg);
>   arg.flags = DRM_MODE_CURSOR_BO;
>   arg.crtc_id = crtcId;
>   arg.width = width;
> @@ -421,6 +422,7 @@ int drmModeSetCursor2(int fd, uint32_t crtcId, uint32_t 
> bo_handle, uint32_t widt
>  {
>   struct drm_mode_cursor2 arg;
>  
> + memclear(arg);
>   arg.flags = DRM_MODE_CURSOR_BO;
>   arg.crtc_id = crtcId;
>   arg.width = width;
> @@ -436,6 +438,7 @@ int drmModeMoveCursor(int fd, uint32_t crtcId, int x, int 
> y)
>  {
>   struct drm_mode_cursor arg;
>  
> + memclear(arg);
>   arg.flags = DRM_MODE_CURSOR_MOVE;
>   arg.crtc_id = crtcId;
>   arg.x = x;
> @@ -452,11 +455,8 @@ drmModeEncoderPtr drmModeGetEncoder(int fd, uint32_t 
> encoder_id)
>   struct drm_mode_get_encoder enc;
>   drmModeEncoderPtr r = NULL;
>  
> + memclear(enc);
>   enc.encoder_id = encoder_id;
> - enc.crtc_id = 0;
> - enc.encoder_type = 0;
> - enc.possible_crtcs = 0;
> - en

Re: [Intel-gfx] [PATCH 1/4] intel: Unconditionally clear ioctl structs

[Ian Romanick]

This patch is

Reviewed-by: Ian Romanick 

On 02/11/2015 03:42 AM, Daniel Vetter wrote:
> We really have to do this to avoid surprises when extending the ABI
> later on. Especially when growing the structures.
> 
> Signed-off-by: Daniel Vetter 
> ---
>  intel/intel_bufmgr_gem.c | 68 
> 
>  1 file changed, 34 insertions(+), 34 deletions(-)
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index cf85bb8ae0bf..78875fd329f2 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -74,7 +74,7 @@
>  #define VG(x)
>  #endif
>  
> -#define VG_CLEAR(s) VG(memset(&s, 0, sizeof(s)))
> +#define memclear(s) memset(&s, 0, sizeof(s))
>  
>  #define DBG(...) do {\
>   if (bufmgr_gem->bufmgr.debug)   \
> @@ -593,7 +593,7 @@ drm_intel_gem_bo_busy(drm_intel_bo *bo)
>   if (bo_gem->reusable && bo_gem->idle)
>   return false;
>  
> - VG_CLEAR(busy);
> + memclear(busy);
>   busy.handle = bo_gem->gem_handle;
>  
>   ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_BUSY, &busy);
> @@ -612,7 +612,7 @@ drm_intel_gem_bo_madvise_internal(drm_intel_bufmgr_gem 
> *bufmgr_gem,
>  {
>   struct drm_i915_gem_madvise madv;
>  
> - VG_CLEAR(madv);
> + memclear(madv);
>   madv.handle = bo_gem->gem_handle;
>   madv.madv = state;
>   madv.retained = 1;
> @@ -741,7 +741,7 @@ retry:
>  
>   bo_gem->bo.size = bo_size;
>  
> - VG_CLEAR(create);
> + memclear(create);
>   create.size = bo_size;
>  
>   ret = drmIoctl(bufmgr_gem->fd,
> @@ -888,7 +888,7 @@ drm_intel_gem_bo_alloc_userptr(drm_intel_bufmgr *bufmgr,
>  
>   bo_gem->bo.size = size;
>  
> - VG_CLEAR(userptr);
> + memclear(userptr);
>   userptr.user_ptr = (__u64)((unsigned long)addr);
>   userptr.user_size = size;
>   userptr.flags = flags;
> @@ -972,7 +972,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr 
> *bufmgr,
>   }
>   }
>  
> - VG_CLEAR(open_arg);
> + memclear(open_arg);
>   open_arg.name = handle;
>   ret = drmIoctl(bufmgr_gem->fd,
>  DRM_IOCTL_GEM_OPEN,
> @@ -1017,7 +1017,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr 
> *bufmgr,
>   bo_gem->global_name = handle;
>   bo_gem->reusable = false;
>  
> - VG_CLEAR(get_tiling);
> + memclear(get_tiling);
>   get_tiling.handle = bo_gem->gem_handle;
>   ret = drmIoctl(bufmgr_gem->fd,
>  DRM_IOCTL_I915_GEM_GET_TILING,
> @@ -1060,7 +1060,7 @@ drm_intel_gem_bo_free(drm_intel_bo *bo)
>   }
>  
>   /* Close this object */
> - VG_CLEAR(close);
> + memclear(close);
>   close.handle = bo_gem->gem_handle;
>   ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_CLOSE, &close);
>   if (ret != 0) {
> @@ -1292,9 +1292,8 @@ static int drm_intel_gem_bo_map(drm_intel_bo *bo, int 
> write_enable)
>   DBG("bo_map: %d (%s), map_count=%d\n",
>   bo_gem->gem_handle, bo_gem->name, bo_gem->map_count);
>  
> - VG_CLEAR(mmap_arg);
> + memclear(mmap_arg);
>   mmap_arg.handle = bo_gem->gem_handle;
> - mmap_arg.offset = 0;
>   mmap_arg.size = bo->size;
>   ret = drmIoctl(bufmgr_gem->fd,
>  DRM_IOCTL_I915_GEM_MMAP,
> @@ -1316,7 +1315,7 @@ static int drm_intel_gem_bo_map(drm_intel_bo *bo, int 
> write_enable)
>   bo_gem->mem_virtual);
>   bo->virtual = bo_gem->mem_virtual;
>  
> - VG_CLEAR(set_domain);
> + memclear(set_domain);
>   set_domain.handle = bo_gem->gem_handle;
>   set_domain.read_domains = I915_GEM_DOMAIN_CPU;
>   if (write_enable)
> @@ -1362,7 +1361,7 @@ map_gtt(drm_intel_bo *bo)
>   DBG("bo_map_gtt: mmap %d (%s), map_count=%d\n",
>   bo_gem->gem_handle, bo_gem->name, bo_gem->map_count);
>  
> - VG_CLEAR(mmap_arg);
> + memclear(mmap_arg);
>   mmap_arg.handle = bo_gem->gem_handle;
>  
>   /* Get the fake offset back... */
> @@ -1430,7 +1429,7 @@ drm_intel_gem_bo_map_gtt(drm_intel_bo *bo)
>* tell it when we're about to use things if we had done
>* rendering and it still happens to be bound to the GTT.
>*/
> - VG_CLEAR(set_domain);
> + memclear(set_domain);
>   

[Intel-gfx] MSR for November / December

[Ian Romanick]

Short version:

- Khronos face-to-face
- BYT performance work

Longer version:

Yet another Khronos face-to-face meeting.  This was a special meeting
just for the gl_common working group to hammer out details of XGL (still
need a name!) so that we can at least have a chance of having a
provisional spec for GDC in March.  We made excellent progress on some
of the tougher issues, and I think there may actually be a chance of
having a usable spec by GDC.

There are still some major sticking points.  From my POV, the biggest
issue is that tile-based renderers (TBR) need some additional
information and / or limitations that immediate-mode renderers (IMR) do
not.  At best an IMR would just drop the data on the floor.  At worst an
IMR would lose performance due to extra state transitions.  On the
second day of the meetings there was a very heated 2 hour "debate" about
this issue.  I think the only thing that came out of it was the obvious
conclusion that app developers need to specifically optimize
applications for TBRs.

While not in meetings or on airplanes, I spent some time looking at
where Mesa spends CPU.  As soon as I started looking, I couldn't not
find problems.  I have about 30 patches of potential micro-optimizations
across the glUniform paths and the draw-time validation paths.  I'd love
to send these out, but I'm having quite a bit of difficulty getting
meaningful performance data to justify the changes.

I have tried several techniques, and I'm not terribly pleased with any
of them.  The most useful have been:

- Use callgrind to get instruction cycle counts.  This provides stable
results, but it's sloow.  It also doesn't account for the
shared memory bus or power management interactions.

- Use a CPU-limited benchmark and measure its framerate.  This provides
full-system data, but the results aren't stable.  This means you have to
do many runs to detect small performance changes.  Very small changes
may just be undetectable.

I wish I had something like shader-db for measuring CPU changes. :(

It's worth noting that some of the SynMark2 timings were completely
botched.  I changed my build scripts to use the same compiler
optimization settings as a distro (I picked Fedora) so that I could get
results more representative of what real users would see.  In the
process, I accidentally left --enable-debug in the configure command.
This left assertions enabled in the code, and, yeah, that affects
performance.

Anyway... a few housekeeping patches have already been sent to mesa-dev,
and the rest should go out before the end of the year.

Next month:

All the travel.  LCA, another Khronos meeting, and FOSDEM.  I'm also
taking a week of vacation in January, and sleeping for most of February.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] intel: Use memset instead of VG_CLEAR

[Ian Romanick]

From: Ian Romanick 

The ioctl expects that certain fields will be zeroed, so we should allow
the helper function to actually work in non-Valgrind builds.

Signed-off-by: Ian Romanick 
Reported-by: Zhenyu Wang 
Cc: Damien Lespiau 
Cc: Daniel Vetter 
---
 intel/intel_bufmgr_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index df6fcec..c11ed45 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -3033,7 +3033,7 @@ drm_intel_get_reset_stats(drm_intel_context *ctx,
if (ctx == NULL)
return -EINVAL;
 
-   VG_CLEAR(stats);
+   memset(&stats, 0, sizeof(stats));
 
bufmgr_gem = (drm_intel_bufmgr_gem *)ctx->bufmgr;
stats.ctx_id = ctx->ctx_id;
-- 
1.8.1.4

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

[Intel-gfx] [PATCH] intel: Add support for GPU reset status query ioctl

[Ian Romanick]

From: Ian Romanick 

I would have just used the drmIoctl interface directly in Mesa, but the
ioctl needs some data from the drm_intel_context that is not exposed
outside libdrm.

This ioctl is in the drm-intel-next tree as b635991.

v2: Update based on Mika's kernel work.

v3: Fix compile failures from last-minute typos.  Sigh.

v4: Import the actual changes from the kernel i915_drm.h.  Only comments
on some fields of drm_i915_reset_stats differed.  There are still some
deltas between the kernel i915_drm.h and the one in libdrm, but those
can be resolved in other patches.

Signed-off-by: Ian Romanick 
Reviewed-by: Kenneth Graunke  [v3]
Cc: Mika Kuoppala 
Cc: Daniel Vetter 
---
 include/drm/i915_drm.h   | 19 +++
 intel/intel_bufmgr.h |  5 +
 intel/intel_bufmgr_gem.c | 34 ++
 3 files changed, 58 insertions(+)

diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index aa983f3..c1914d6 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -198,6 +198,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_SET_CACHEING  0x2f
 #define DRM_I915_GEM_GET_CACHEING  0x30
 #define DRM_I915_REG_READ  0x31
+#define DRM_I915_GET_RESET_STATS   0x32
 
 #define DRM_IOCTL_I915_INITDRM_IOW( DRM_COMMAND_BASE + 
DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH   DRM_IO ( DRM_COMMAND_BASE + 
DRM_I915_FLUSH)
@@ -247,6 +248,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE  DRM_IOWR (DRM_COMMAND_BASE + 
DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
 #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY DRM_IOW (DRM_COMMAND_BASE + 
DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
 #define DRM_IOCTL_I915_REG_READDRM_IOWR 
(DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
+#define DRM_IOCTL_I915_GET_RESET_STATS DRM_IOWR (DRM_COMMAND_BASE + 
DRM_I915_GET_RESET_STATS, struct drm_i915_reset_stats)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -943,4 +945,21 @@ struct drm_i915_reg_read {
__u64 offset;
__u64 val; /* Return value */
 };
+
+struct drm_i915_reset_stats {
+   __u32 ctx_id;
+   __u32 flags;
+
+   /* All resets since boot/module reload, for all contexts */
+   __u32 reset_count;
+
+   /* Number of batches lost when active in GPU, for this context */
+   __u32 batch_active;
+
+   /* Number of batches lost pending for execution, for this context */
+   __u32 batch_pending;
+
+   __u32 pad;
+};
+
 #endif /* _I915_DRM_H_ */
diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
index 7b28a70..34a5750 100644
--- a/intel/intel_bufmgr.h
+++ b/intel/intel_bufmgr.h
@@ -249,6 +249,11 @@ int drm_intel_reg_read(drm_intel_bufmgr *bufmgr,
   uint32_t offset,
   uint64_t *result);
 
+int drm_intel_get_reset_stats(drm_intel_context *ctx,
+ uint32_t *reset_count,
+ uint32_t *active,
+ uint32_t *pending);
+
 /** @{ Compatibility defines to keep old code building despite the symbol 
rename
  * from dri_* to drm_intel_*
  */
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index dbadc52..0b9252e 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -3029,6 +3029,40 @@ drm_intel_gem_context_get_hw_context_id(const 
drm_intel_context *ctx)
 }
 
 int
+drm_intel_get_reset_stats(drm_intel_context *ctx,
+ uint32_t *reset_count,
+ uint32_t *active,
+ uint32_t *pending)
+{
+   drm_intel_bufmgr_gem *bufmgr_gem;
+   struct drm_i915_reset_stats stats;
+   int ret;
+
+   if (ctx == NULL)
+   return -EINVAL;
+
+   VG_CLEAR(stats);
+
+   bufmgr_gem = (drm_intel_bufmgr_gem *)ctx->bufmgr;
+   stats.ctx_id = ctx->ctx_id;
+   ret = drmIoctl(bufmgr_gem->fd,
+  DRM_IOCTL_I915_GET_RESET_STATS,
+  &stats);
+   if (ret == 0) {
+   if (reset_count != NULL)
+   *reset_count = stats.reset_count;
+
+   if (active != NULL)
+   *active = stats.batch_active;
+
+   if (pending != NULL)
+   *pending = stats.batch_pending;
+   }
+
+   return ret;
+}
+
+int
 drm_intel_reg_read(drm_intel_bufmgr *bufmgr,
   uint32_t offset,
   uint64_t *result)
-- 
1.8.1.4

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

Re: [Intel-gfx] [PATCH 2/2] drm/i915: add i915_get_reset_stats_ioctl

[Ian Romanick]

On 11/08/2013 10:11 AM, Damien Lespiau wrote:
> On Wed, Oct 30, 2013 at 03:44:16PM +0200, Mika Kuoppala wrote:
>> This ioctl returns reset stats for specified context.
>>
>> The struct returned contains context loss counters.
>>
>> reset_count:all resets across all contexts
>> batch_active:   active batches lost on resets
>> batch_pending:  pending batches lost on resets
>>
>> v2: get rid of state tracking completely and deliver only counts. Idea
>> from Chris Wilson.
>>
>> v3: fix commit message
>>
>> v4: default context handled inside i915_gem_context_get_hang_stats
>>
>> v5: reset_count only for priviledged process
>>
>> v6: ctx=0 needs CAP_SYS_ADMIN for batch_* counters (Chris Wilson)
>>
>> v7: context hang stats never returns NULL
>>
>> v8: rebased on top of reworked context hang stats
>> DRM_RENDER_ALLOW for ioctl
>>
>> v9: use DEFAULT_CONTEXT_ID. Improve comments for ioctl struct members
>>
>> Signed-off-by: Mika Kuoppala 
>> Cc: Ian Romanick 
>> Cc: Chris Wilson 
>> Cc: Daniel Vetter 
> 
> The logic there looks good to me, I was just wondering if we wanted a
> bit more padding in case we want other counters or misc stuff.

I think the current padding should be sufficient.  The other things that
could occur aren't things that we should return counts of.  For those
kinds of things, we'd only want to return flags.  In fact, the current
user space only uses the counts as flags anyway (is the count non-zero?).

> Reviewed-by: Damien Lespiau 

Also,

Reviewed-by: Ian Romanick 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/2] intel: Add accessor to get HW context ID from a drm_intel_context

[Ian Romanick]

From: Ian Romanick 

The drm_intel_context structure is, wisely, opaque.  However, libdrm
users may want to know the hardware context ID associated with the
structure.

Signed-off-by: Ian Romanick 
Cc: Ben Widawsky 
---
 intel/intel_bufmgr.h | 1 +
 intel/intel_bufmgr_gem.c | 6 ++
 2 files changed, 7 insertions(+)

diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
index 15f818e..7b28a70 100644
--- a/intel/intel_bufmgr.h
+++ b/intel/intel_bufmgr.h
@@ -194,6 +194,7 @@ drm_intel_context 
*drm_intel_gem_context_create(drm_intel_bufmgr *bufmgr);
 void drm_intel_gem_context_destroy(drm_intel_context *ctx);
 int drm_intel_gem_bo_context_exec(drm_intel_bo *bo, drm_intel_context *ctx,
  int used, unsigned int flags);
+unsigned int drm_intel_gem_context_get_hw_context_id(const drm_intel_context 
*);
 
 int drm_intel_bo_gem_export_to_prime(drm_intel_bo *bo, int *prime_fd);
 drm_intel_bo *drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr,
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 029ca5d..5b64a7f 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -3020,6 +3020,12 @@ drm_intel_gem_context_destroy(drm_intel_context *ctx)
free(ctx);
 }
 
+unsigned int
+drm_intel_gem_context_get_hw_context_id(const drm_intel_context *ctx)
+{
+   return ctx->ctx_id;
+}
+
 int
 drm_intel_reg_read(drm_intel_bufmgr *bufmgr,
   uint32_t offset,
-- 
1.8.1.4

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

[Intel-gfx] [PATCH 2/2] intel: Silence warning in non-Valgrind build

[Ian Romanick]

From: Ian Romanick 

Previously GCC was squaking about:

intel_bufmgr_gem.c: In function 'drm_intel_gem_bo_map_unsynchronized':
intel_bufmgr_gem.c:1325:20: warning: unused variable 'bo_gem' 
[-Wunused-variable]

Wrapping with VG(); replaced that warning with:

intel_bufmgr_gem.c: In function 'drm_intel_gem_bo_map_unsynchronized':
intel_bufmgr_gem.c:1326:2: warning: ISO C90 forbids mixed declarations and code 
[-Wdeclaration-after-statement]

Which, looking at the definition of the VG macro, seems a bit weird.
It's caused by the dangling ";" left from the empty macro.

Signed-off-by: Ian Romanick 
Cc: Chia-I Wu 
---
 intel/intel_bufmgr_gem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 5b64a7f..dbadc52 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -1322,7 +1322,9 @@ int drm_intel_gem_bo_map_gtt(drm_intel_bo *bo)
 int drm_intel_gem_bo_map_unsynchronized(drm_intel_bo *bo)
 {
drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
+#ifdef HAVE_VALGRIND
drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
+#endif
int ret;
 
/* If the CPU cache isn't coherent with the GTT, then use a
-- 
1.8.1.4

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

Re: [Intel-gfx] [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ian Romanick]

On 11/07/2013 10:32 PM, Daniel Vetter wrote:
> On Fri, Nov 8, 2013 at 6:48 AM, Dave Airlie  wrote:
>> On Wed, Oct 30, 2013 at 6:21 PM, Daniel Vetter  wrote:
>>> On Tue, Oct 29, 2013 at 11:29 PM, Ian Romanick  wrote:
>>>> On 10/27/2013 05:30 AM, Daniel Vetter wrote:
>>>>> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
>>>>>> Since the Mesa merge window is closing soon, I'm finally getting back on
>>>>>> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
>>>>>>
>>>>>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
>>>>>>
>>>>>> I have a couple questions...
>>>>>>
>>>>>> 1. Has any of this landed an a kernel tree anywhere?
>>>>>
>>>>> Afaik everything but the actual ioctl and i-g-t testcase has landed.
>>>>
>>>> And that stuff will land once my patches hit the Mesa list or ... ?
>>>
>>> Yup.
>>
>> Hey kernel first, then upstream projects, at the moment libdrm has
>> ioctls in it that I have no upstream solid kernel commit for,
>>
>> Either in the next 24 hrs I have this in my tree or the libdrm commits
>> need to be reverted,
>>
>> and if someone releases libdrm in that time span then I'm going to be
>> quite pissed.
> 
> It's kinda too late imo for 3.13 (and there's an open question whether
> we need one more flag or not), so I wanted to pull it in into 3.14.
> Which also gives us plenty of time to add or not add that optional
> flag. So I guess time to revert. Can you do that pls?

Reverting has completely broken Mesa builds, and was the wrong choice.
Thanks for giving me opportunity to reply before breaking my stuff.

> -Daniel

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

Re: [Intel-gfx] [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ian Romanick]

On 11/08/2013 11:00 AM, Dave Airlie wrote:
> On Sat, Nov 9, 2013 at 3:21 AM, Ian Romanick  wrote:
>> On 11/07/2013 10:32 PM, Daniel Vetter wrote:
>>> On Fri, Nov 8, 2013 at 6:48 AM, Dave Airlie  wrote:
>>>> On Wed, Oct 30, 2013 at 6:21 PM, Daniel Vetter  wrote:
>>>>> On Tue, Oct 29, 2013 at 11:29 PM, Ian Romanick  
>>>>> wrote:
>>>>>> On 10/27/2013 05:30 AM, Daniel Vetter wrote:
>>>>>>> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
>>>>>>>> Since the Mesa merge window is closing soon, I'm finally getting back 
>>>>>>>> on
>>>>>>>> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
>>>>>>>>
>>>>>>>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
>>>>>>>>
>>>>>>>> I have a couple questions...
>>>>>>>>
>>>>>>>> 1. Has any of this landed an a kernel tree anywhere?
>>>>>>>
>>>>>>> Afaik everything but the actual ioctl and i-g-t testcase has landed.
>>>>>>
>>>>>> And that stuff will land once my patches hit the Mesa list or ... ?
>>>>>
>>>>> Yup.
>>>>
>>>> Hey kernel first, then upstream projects, at the moment libdrm has
>>>> ioctls in it that I have no upstream solid kernel commit for,
>>>>
>>>> Either in the next 24 hrs I have this in my tree or the libdrm commits
>>>> need to be reverted,
>>>>
>>>> and if someone releases libdrm in that time span then I'm going to be
>>>> quite pissed.
>>>
>>> It's kinda too late imo for 3.13 (and there's an open question whether
>>> we need one more flag or not), so I wanted to pull it in into 3.14.
>>> Which also gives us plenty of time to add or not add that optional
>>> flag. So I guess time to revert. Can you do that pls?
>>
>> Reverting has completely broken Mesa builds, and was the wrong choice.
>> Thanks for giving me opportunity to reply before breaking my stuff.
> 
> Hey Ian,
> 
> stop merging incomplete shit 5 mins before the branch point,

Stop calling other people's hard work shit.  It just makes you look like
an asshole.  Seriously.

> you should know better, Mesa is meant to be moving to 3 mth release
> cycle to avoid this kinda merge everything crap,

That's not the reason for the 3 month cycle.  The reason for the 3 month
cycle is because all of the distros, including Fedora, were pulling
random points from master because waiting 6 months for features and
performance improvements was too long.

> there was an open thread on the api for this feature, there was
> questions of whether a drm cap was needed, you failed to address any

I don't know where you're getting "drm cap" nonsense.  That was closed
two weeks ago:

http://lists.freedesktop.org/archives/intel-gfx/2013-October/035016.html

> of these and you merged the feature, the rules aren't different than
> before, stuff goes in the kernel first and userspace second, we went
> down this road before and it always gets screwed up.

I thought Daniel and I had closed that, and I Acked-by the kernel patch.
 He and I talked about this on IRC.  ***IF*** we need an extra flag for
global resets, there's already a place for it in the (currently) pad
field returned by the ioctl.

This isn't some last minute, half-baked interface.  We've been working
on this for nearly a year.  After going through several revisions,
Mika's kernel patch has been on the intel-gfx list since July.  That
people are exploding about this now just boggles my mind.

> Dave.

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

Re: [Intel-gfx] [RFC/Draft] Testing requirements for upstream drm/i915 patches

[Ian Romanick]

On 10/30/2013 09:00 AM, Daniel Vetter wrote:
> [This is cross-posted to the public intel-gfx mailing list at
> http://lists.freedesktop.org/archives/intel-gfx/2013-October/035268.html
> I'll also present a quick overview of this at Gavin's kernel PDT next
> week.]
> 
> Hi all,
> 
> So in the past half year we've had tons of sometimes rather heated
> discussions
> about getting patches merged. Often these discussions have been in the
> context
> of specific patch series, which meant that people are already invested.
> Which
> contributed to the boiling emotions. I'd like to avoid that here by
> making this
> a free-standing discussion.
> 
> There's a bunch of smaller process tuning going on, but the big thing
> I'd like
> to instate henceforth is that automated test coverage is a primary
> consideration
> for anything going upstream. In this write up I'll detail my reasons,
> considerations and expectations. My plan is to solicit feedback over the
> next
> few days and then publish an edited and polished version to my blog.
> 
> After that I'll put down my foot on this process so that we can go back to
> coding and stop blowing through so much time and energy on waging
> flamewars.
> 
> Feedback and critique highly welcome.
> 
> Cheers, Daniel
> 
> Testing Requirements for Upstreaming (Draft)
> 
> 
> I want to make automated test coverage an integral part of our feature
> and bufix
  ^ bugfix

> development process. For features this means that starting with the
> design phase
> testability needs to be considered an integral part of any feature. This
> needs
> to go up through the entire development process until when the
> implementation is
> submitted together with the proposed tests. For bugfixes that means the
> fix is
> only complete once the automated testcase for it is also done, if we
> need a new
> one.
> 
> This specifically excludes testing with humans somewhere in the loop. We
> are
> extremely limited in our validation resources, every time we put
> something new
> onto the "manual testing" plate something else _will_ fall off.
> 
> Why?
> 
> 
> - More predictability. Right now test coverage often only comes up as a
> topic
>   when I drop my maintainer review onto a patch series. Which is too
> late, since
>   it'll delay the otherwise working patches and so massively frustrates
> people.
>   I hope by making test requirements clear and up-front we can make the
>   upstreaming process more predictable. Also, if we have good tests from
> the get-go
>   there should be much less need for me to drop patches from my trees
>   after having them merged.
> 
> - Less bikeshedding. In my opinion test cases are an excellent means to
> settle
>   bikesheds - we've had in the past seen cases of endless back&forths
>   where writing a simple testcase would have shown that _all_ proposed
>   color flavours are actually broken.
> 
>   The even more important thing is that fully automated tests allow us to
>   legitimately postpone cleanups. If the only testing we have is manual
> testing
>   then we have only one shot at a feature tested, namely when the developer
>   tests it. So it better be perfect. But with automated tests we can
> postpone
>   cleanups with too high risks of regressions until a really clear need is
>   established. And since that need often never materializes we'll save
> work.
> 
> - Better review. For me it's often helps a lot to review tests than the
> actual
>   code in-depth. This is especially true for reviewing userspace interface
>   additions.
> 
> - Actionable regression reports. Only if we have a fully automated
> testcase do
>   we have a good chance that QA reports a regression within just a few
> days.
>   Everything else can easily take weeks (for platforms and features
> which are
>   explicitly tested) to months (for stuff only users from the community
> notice).
>   And especially now that much more shipping products depend upon a working
>   i915.ko driver we just can't do this any more.
> 
> - Better tests. A lot of our code is really hard to test in an automated
>   fashion, and pushing the frontier of what is testable often requires a
> lot of
>   work. I hope that by making tests an integral part of any feature work
> and so
>   forcing more people to work on them and think about testing we'll
>   advance the state of the art at a brisker pace.
> 
> Risks and Buts
> --
> 
> - Bikeshedding on tests. This plan is obviously not too useful if we just
>   replace massive bikeshedding on patches with massive bikeshedding on
>   testcases. But right now we do almost no review on i-g-t patches so
> the risk
>   is small. Long-term the review requirements for testcases will certainly
>   increase, but as with everything else we simply need to strive for a good
>   balance to strike for just the right amount of review.
> 
>   Also if we really start discussing tests _before_ having written
> massive patch
>

Re: [Intel-gfx] [PATCH 2/2] drm/i915: add i915_get_reset_stats_ioctl

[Ian Romanick]

On 10/30/2013 06:44 AM, Mika Kuoppala wrote:
> This ioctl returns reset stats for specified context.
> 
> The struct returned contains context loss counters.
> 
> reset_count:all resets across all contexts
> batch_active:   active batches lost on resets
> batch_pending:  pending batches lost on resets
> 
> v2: get rid of state tracking completely and deliver only counts. Idea
> from Chris Wilson.
> 
> v3: fix commit message
> 
> v4: default context handled inside i915_gem_context_get_hang_stats
> 
> v5: reset_count only for priviledged process
> 
> v6: ctx=0 needs CAP_SYS_ADMIN for batch_* counters (Chris Wilson)
> 
> v7: context hang stats never returns NULL
> 
> v8: rebased on top of reworked context hang stats
> DRM_RENDER_ALLOW for ioctl
> 
> v9: use DEFAULT_CONTEXT_ID. Improve comments for ioctl struct members
> 
> Signed-off-by: Mika Kuoppala 
> Cc: Ian Romanick 
> Cc: Chris Wilson 
> Cc: Daniel Vetter 
> ---
>  drivers/gpu/drm/i915/i915_dma.c |1 +
>  drivers/gpu/drm/i915/i915_drv.h |2 ++
>  drivers/gpu/drm/i915/intel_uncore.c |   34 ++
>  include/uapi/drm/i915_drm.h |   19 +++
>  4 files changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 6eecce7..f2cdeb2 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1921,6 +1921,7 @@ const struct drm_ioctl_desc i915_ioctls[] = {
>   DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, 
> i915_gem_context_create_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>   DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, 
> i915_gem_context_destroy_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>   DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, 
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl, 
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
>  };
>  
>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9fd716d..8870804 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2369,6 +2369,8 @@ extern int intel_enable_rc6(const struct drm_device 
> *dev);
>  extern bool i915_semaphore_is_enabled(struct drm_device *dev);
>  int i915_reg_read_ioctl(struct drm_device *dev, void *data,
>   struct drm_file *file);
> +int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
> +struct drm_file *file);
>  
>  /* overlay */
>  extern struct intel_overlay_error_state 
> *intel_overlay_capture_error_state(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
> b/drivers/gpu/drm/i915/intel_uncore.c
> index f6fae35..21cf951 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -633,6 +633,40 @@ int i915_reg_read_ioctl(struct drm_device *dev,
>   return 0;
>  }
>  
> +int i915_get_reset_stats_ioctl(struct drm_device *dev,
> +void *data, struct drm_file *file)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_i915_reset_stats *args = data;
> + struct i915_ctx_hang_stats *hs;
> + int ret;
> +
> + if (args->ctx_id == DEFAULT_CONTEXT_ID && !capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + ret = mutex_lock_interruptible(&dev->struct_mutex);
> + if (ret)
> + return ret;
> +
> + hs = i915_gem_context_get_hang_stats(dev, file, args->ctx_id);
> + if (IS_ERR(hs)) {
> + mutex_unlock(&dev->struct_mutex);
> + return PTR_ERR(hs);
> + }
> +
> + if (capable(CAP_SYS_ADMIN))
> + args->reset_count = i915_reset_count(&dev_priv->gpu_error);
> + else
> + args->reset_count = 0;

We're having some additional debate about issues related to this.  Eric
(added to CC so he'll notice) believes that we may encounter memory
corruption around a reset (most likely causing the reset instead of the
other way around).  This means that we may need to deliver a reset
notification to an otherwise unaffected GL context after all. :(

If we decided that this is possible, we should deliver a single bit to
user mode that says "there was a reset after this context was created."
 I assume that could be returned to user space in the flags field?

I don't think this provides the same potential information leak as
directly exposing the global reset count, but I could be wrong.

I don't think we need to change 

Re: [Intel-gfx] [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ian Romanick]

On 10/27/2013 05:30 AM, Daniel Vetter wrote:
> On Fri, Oct 25, 2013 at 06:42:35PM -0700, Ian Romanick wrote:
>> Since the Mesa merge window is closing soon, I'm finally getting back on
>> this.  I've pushed a rebase of my old Mesa branch to my fd.o repo
>>
>> http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3
>>
>> I have a couple questions...
>>
>> 1. Has any of this landed an a kernel tree anywhere?
> 
> Afaik everything but the actual ioctl and i-g-t testcase has landed.

And that stuff will land once my patches hit the Mesa list or ... ?

>> 2. Has any support code landed in a libdrm tree anywhere?
> 
> Dunno whether Mika has libdrm patches. Since mesa is the only expected
> user I'd just go with putting the ioctl wrapper (using the drmIoctl
> helper) into mesa itself, that get rids of a dep for merging this support.

What's the right way to get the ctx_id out of the drm_intel_context?
That struct is private to libdrm, but the ioctl needs it.

>> 3. What method should I use to detect that the kernel has support?  In
>> early discussions, reset notification was only going to be available on
>> some GPUs, so there was a getparam to detect actual availability.  I
>> guess now it's just based on kernel version?
> 
> Usually we add a new feature flag to get get_param ioctl if there's no
> natural way otherwise for userspace to figure this out (usually by calling
> the new ioctl and disabling the feature if that doesn't work).
> -Daniel
> 
>>
>> It looks like I should just need to update df87cdd and 61dad8e in my
>> Mesa tree.
>>
>> On 07/03/2013 07:22 AM, Mika Kuoppala wrote:
>>> This ioctl returns reset stats for specified context.
>>>
>>> The struct returned contains context loss counters.
>>>
>>> reset_count:all resets across all contexts
>>> batch_active:   active batches lost on resets
>>> batch_pending:  pending batches lost on resets
>>>
>>> v2: get rid of state tracking completely and deliver only counts. Idea
>>> from Chris Wilson.
>>>
>>> v3: fix commit message
>>>
>>> v4: default context handled inside i915_gem_contest_get_hang_stats
>>>
>>> v5: reset_count only for priviledged process
>>>
>>> v6: ctx=0 needs CAP_SYS_ADMIN for batch_* counters (Chris Wilson)
>>>
>>> v7: context hang stats never returns NULL
>>>
>>> Signed-off-by: Mika Kuoppala 
>>> Cc: Ian Romanick 
>>> Cc: Chris Wilson 
>>> Cc: Daniel Vetter 
>>> ---
>>>  drivers/gpu/drm/i915/i915_dma.c |1 +
>>>  drivers/gpu/drm/i915/i915_drv.c |   34 ++
>>>  drivers/gpu/drm/i915/i915_drv.h |2 ++
>>>  include/uapi/drm/i915_drm.h |   17 +
>>>  4 files changed, 54 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c 
>>> b/drivers/gpu/drm/i915/i915_dma.c
>>> index 0e22142..d1a006f 100644
>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>> @@ -1889,6 +1889,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
>>> DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, 
>>> i915_gem_context_create_ioctl, DRM_UNLOCKED),
>>> DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, 
>>> i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
>>> DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
>>> +   DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl, 
>>> DRM_UNLOCKED),
>>>  };
>>>  
>>>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> index 33cb973..0d4e3a8 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -1350,3 +1350,37 @@ int i915_reg_read_ioctl(struct drm_device *dev,
>>>  
>>> return 0;
>>>  }
>>> +
>>> +int i915_get_reset_stats_ioctl(struct drm_device *dev,
>>> +  void *data, struct drm_file *file)
>>> +{
>>> +   struct drm_i915_private *dev_priv = dev->dev_private;
>>> +   struct drm_i915_reset_stats *args = data;
>>> +   struct i915_ctx_hang_stats *hs;
>>> +   int ret;
>>> +
>>> +   if (args->ctx_id == 0 && !capable(CAP_SYS_ADMIN))
>>> +   return -EPERM;
>>> +
>>> +   ret = mutex_lock_interruptible(&dev->struct_mutex);

Re: [Intel-gfx] [PATCH 7/7] drm/i915: add i915_get_reset_stats_ioctl

[Ian Romanick]

Since the Mesa merge window is closing soon, I'm finally getting back on
this.  I've pushed a rebase of my old Mesa branch to my fd.o repo

http://cgit.freedesktop.org/~idr/mesa/log/?h=robustness3

I have a couple questions...

1. Has any of this landed an a kernel tree anywhere?

2. Has any support code landed in a libdrm tree anywhere?

3. What method should I use to detect that the kernel has support?  In
early discussions, reset notification was only going to be available on
some GPUs, so there was a getparam to detect actual availability.  I
guess now it's just based on kernel version?

It looks like I should just need to update df87cdd and 61dad8e in my
Mesa tree.

On 07/03/2013 07:22 AM, Mika Kuoppala wrote:
> This ioctl returns reset stats for specified context.
> 
> The struct returned contains context loss counters.
> 
> reset_count:all resets across all contexts
> batch_active:   active batches lost on resets
> batch_pending:  pending batches lost on resets
> 
> v2: get rid of state tracking completely and deliver only counts. Idea
> from Chris Wilson.
> 
> v3: fix commit message
> 
> v4: default context handled inside i915_gem_contest_get_hang_stats
> 
> v5: reset_count only for priviledged process
> 
> v6: ctx=0 needs CAP_SYS_ADMIN for batch_* counters (Chris Wilson)
> 
> v7: context hang stats never returns NULL
> 
> Signed-off-by: Mika Kuoppala 
> Cc: Ian Romanick 
> Cc: Chris Wilson 
> Cc: Daniel Vetter 
> ---
>  drivers/gpu/drm/i915/i915_dma.c |1 +
>  drivers/gpu/drm/i915/i915_drv.c |   34 ++
>  drivers/gpu/drm/i915/i915_drv.h |2 ++
>  include/uapi/drm/i915_drm.h |   17 +
>  4 files changed, 54 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 0e22142..d1a006f 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1889,6 +1889,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
>   DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, 
> i915_gem_context_create_ioctl, DRM_UNLOCKED),
>   DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, 
> i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
>   DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
> + DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl, 
> DRM_UNLOCKED),
>  };
>  
>  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 33cb973..0d4e3a8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1350,3 +1350,37 @@ int i915_reg_read_ioctl(struct drm_device *dev,
>  
>   return 0;
>  }
> +
> +int i915_get_reset_stats_ioctl(struct drm_device *dev,
> +void *data, struct drm_file *file)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_i915_reset_stats *args = data;
> + struct i915_ctx_hang_stats *hs;
> + int ret;
> +
> + if (args->ctx_id == 0 && !capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + ret = mutex_lock_interruptible(&dev->struct_mutex);
> + if (ret)
> + return ret;
> +
> + hs = i915_gem_context_get_hang_stats(dev, file, args->ctx_id);
> + if (IS_ERR(hs)) {
> + mutex_unlock(&dev->struct_mutex);
> + return PTR_ERR(hs);
> + }
> +
> + if (capable(CAP_SYS_ADMIN))
> + args->reset_count = i915_reset_count(&dev_priv->gpu_error);
> + else
> + args->reset_count = 0;
> +
> + args->batch_active = hs->batch_active;
> + args->batch_pending = hs->batch_pending;
> +
> + mutex_unlock(&dev->struct_mutex);
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1def049..0ca98fa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2021,6 +2021,8 @@ extern int intel_enable_rc6(const struct drm_device 
> *dev);
>  extern bool i915_semaphore_is_enabled(struct drm_device *dev);
>  int i915_reg_read_ioctl(struct drm_device *dev, void *data,
>   struct drm_file *file);
> +int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
> +struct drm_file *file);
>  
>  /* overlay */
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 923ed7f..29b07fd 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -198,6 +198,7 @@ typedef struct _

Re: [Intel-gfx] [PATCH] intel: Update i915_drm.h and correct misspelled caching

[Ian Romanick]

On 08/14/2013 01:19 AM, Sedat Dilek wrote:

AFAICS, there are more updates needed to be in sync with recent kernel-drm.

I fell over the misspelling when digging into an issue in Linux-next.
The spelling should be consistent in kernel-drm, libdrm, intel-ddx, etc.
Here, I had a look especially at the defined macros (defines).

Signed-off-by: Sedat Dilek 


This should get Chris's ok before committing, but

Reviewed-by: Ian Romanick 
---
  include/drm/i915_drm.h | 21 +++--
  1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index aa983f3..61a8407 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -195,8 +195,8 @@ typedef struct _drm_i915_sarea {
  #define DRM_I915_GEM_WAIT 0x2c
  #define DRM_I915_GEM_CONTEXT_CREATE   0x2d
  #define DRM_I915_GEM_CONTEXT_DESTROY  0x2e
-#define DRM_I915_GEM_SET_CACHEING  0x2f
-#define DRM_I915_GEM_GET_CACHEING  0x30
+#define DRM_I915_GEM_SET_CACHING   0x2f
+#define DRM_I915_GEM_GET_CACHING   0x30
  #define DRM_I915_REG_READ 0x31

  #define DRM_IOCTL_I915_INIT   DRM_IOW( DRM_COMMAND_BASE + 
DRM_I915_INIT, drm_i915_init_t)
@@ -222,8 +222,8 @@ typedef struct _drm_i915_sarea {
  #define DRM_IOCTL_I915_GEM_PINDRM_IOWR(DRM_COMMAND_BASE + 
DRM_I915_GEM_PIN, struct drm_i915_gem_pin)
  #define DRM_IOCTL_I915_GEM_UNPIN  DRM_IOW(DRM_COMMAND_BASE + 
DRM_I915_GEM_UNPIN, struct drm_i915_gem_unpin)
  #define DRM_IOCTL_I915_GEM_BUSY   DRM_IOWR(DRM_COMMAND_BASE + 
DRM_I915_GEM_BUSY, struct drm_i915_gem_busy)
-#define DRM_IOCTL_I915_GEM_SET_CACHEING
DRM_IOW(DRM_COMMAND_BASE + DRM_I915_GEM_SET_CACHEING, struct 
drm_i915_gem_cacheing)
-#define DRM_IOCTL_I915_GEM_GET_CACHEING
DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_GET_CACHEING, struct 
drm_i915_gem_cacheing)
+#define DRM_IOCTL_I915_GEM_SET_CACHING DRM_IOW(DRM_COMMAND_BASE + 
DRM_I915_GEM_SET_CACHING, struct drm_i915_gem_caching)
+#define DRM_IOCTL_I915_GEM_GET_CACHING DRM_IOWR(DRM_COMMAND_BASE + 
DRM_I915_GEM_GET_CACHING, struct drm_i915_gem_caching)
  #define DRM_IOCTL_I915_GEM_THROTTLE   DRM_IO ( DRM_COMMAND_BASE + 
DRM_I915_GEM_THROTTLE)
  #define DRM_IOCTL_I915_GEM_ENTERVTDRM_IO(DRM_COMMAND_BASE + 
DRM_I915_GEM_ENTERVT)
  #define DRM_IOCTL_I915_GEM_LEAVEVTDRM_IO(DRM_COMMAND_BASE + 
DRM_I915_GEM_LEAVEVT)
@@ -706,21 +706,22 @@ struct drm_i915_gem_busy {
__u32 busy;
  };

-#define I915_CACHEING_NONE 0
-#define I915_CACHEING_CACHED   1
+#define I915_CACHING_NONE  0
+#define I915_CACHING_CACHED1
+#define I915_CACHING_DISPLAY   2

-struct drm_i915_gem_cacheing {
+struct drm_i915_gem_caching {
/**
-* Handle of the buffer to set/get the cacheing level of. */
+* Handle of the buffer to set/get the caching level of. */
__u32 handle;

/**
 * Cacheing level to apply or return value
 *
-* bits0-15 are for generic cacheing control (i.e. the above defined
+* bits0-15 are for generic caching control (i.e. the above defined
 * values). bits16-31 are reserved for platform-specific variations
 * (e.g. l3$ caching on gen7). */
-   __u32 cacheing;
+   __u32 caching;
  };

  #define I915_TILING_NONE  0



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

Re: [Intel-gfx] [PATCH v2 16/16] drm/i915: add i915_gem_context_get_reset_status_ioctl

[Ian Romanick]

On 03/19/2013 05:58 AM, Mika Kuoppala wrote:

Ian Romanick  writes:


On 03/14/2013 08:52 AM, Mika Kuoppala wrote:

This ioctl returns context reset status for specified context.

Signed-off-by: Mika Kuoppala 
CC: i...@freedesktop.org
---
   drivers/gpu/drm/i915/i915_dma.c |1 +
   drivers/gpu/drm/i915/i915_drv.c |   61 
+++
   drivers/gpu/drm/i915/i915_drv.h |2 ++
   include/uapi/drm/i915_drm.h |   28 ++
   4 files changed, 92 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7902d97..c919832 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1903,6 +1903,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, 
i915_gem_context_create_ioctl, DRM_UNLOCKED),
DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, 
i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
+   DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATUS, 
i915_gem_context_get_reset_status_ioctl, DRM_UNLOCKED),
   };

   int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 69c9856..a4d06f2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1267,3 +1267,64 @@ int i915_reg_read_ioctl(struct drm_device *dev,

return 0;
   }
+
+int i915_gem_context_get_reset_status_ioctl(struct drm_device *dev,
+   void *data, struct drm_file *file)
+{
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   struct intel_ring_buffer *ring;
+   struct drm_i915_reset_status *args = data;
+   struct ctx_reset_state *rs = NULL;
+   unsigned long reset_cnt;
+   u32 reset_status = I915_RESET_UNKNOWN;
+   int ret;
+
+   ret = mutex_lock_interruptible(&dev->struct_mutex);
+   if (ret)
+   return ret;
+
+   ring = &dev_priv->ring[RCS];
+
+   ret = i915_gem_context_get_reset_state(ring,
+  file,
+  args->ctx_id,
+  &rs);
+   if (ret)
+   goto out;
+
+   BUG_ON(!rs);
+
+   reset_cnt = atomic_read(&dev_priv->gpu_error.reset_counter);
+
+   if (reset_cnt & I915_RESET_IN_PROGRESS_FLAG ||


In this case, I believe we're supposed to return the reset state to the
application.  The ARB_robustness spec says:

  "If a reset status other than NO_ERROR is returned and subsequent
  calls return NO_ERROR, the context reset was encountered and
  completed. If a reset status is repeatedly returned, the context may
  be in the process of resetting."

If the reset takes a long time, it seems that even a well-behaved app
could run afoul of the 'banned' logic.


As there reset status is initialized to I915_RESET_UNKNOWN,
we return it if the reset is in progress or gpu is wedged.


Hmm... so user space will see I915_RESET_UNKNOWN until the reset is 
done, then it will (usually) see either I915_RESET_BATCH_ACTIVE or 
I915_RESET_BATCH_PENDING.  I think that should be okay.



+   reset_cnt == I915_WEDGED) {
+   goto out;
+   }
+
+   /* Set guilty/innocent status if only one reset was
+* observed and if only one guilty was found
+*/
+   if ((rs->reset_cnt + 2) == reset_cnt &&
+   (rs->guilty_cnt + 1) == dev_priv->gpu_error.guilty_cnt) {


This logic seems... wrong, or at least weird.  "rs->reset_cnt + 2" is
confusing next to "if only one reset was observed".

dev_priv->gpu_error.reset_counter is the global GPU reset count since
start-up, and rs->reset_cnt is the global GPU count since start-up when
the context was created.  Right?


Right. The confusing part in here is the
dev_priv->gpu_error.reset_counter. If it is odd, reset is in progress,
if it is even, the reset has been handled and all is well. That is why +2


That's a clever hack, I'm assuming, to use atomic operations instead of 
locks.   Dear God that's awful to understand... it's a tiny bit more 
clear looking back at the 'reset_cnt & I915_RESET_IN_PROGRESS_FLAG'. 
Perhaps we could get some wrapper macros RESET_IN_PROGRESS() and 
RESET_ACTUAL_COUNT() or something?



If that's the case, this will cause a context that was completely idle
(i.e., didn't actually lose anything) to get a reset notification.
That's an absolute deal breaker.


This was then misunderstood by me. I will make it so that if you have
no batches submitted, you wont observe a reset.


If that's not the case, then this architecture needs a lot more
documentation so that people new to it 

Re: [Intel-gfx] [PATCH v2 16/16] drm/i915: add i915_gem_context_get_reset_status_ioctl

[Ian Romanick]

On 03/14/2013 08:52 AM, Mika Kuoppala wrote:

This ioctl returns context reset status for specified context.

Signed-off-by: Mika Kuoppala 
CC: i...@freedesktop.org
---
  drivers/gpu/drm/i915/i915_dma.c |1 +
  drivers/gpu/drm/i915/i915_drv.c |   61 +++
  drivers/gpu/drm/i915/i915_drv.h |2 ++
  include/uapi/drm/i915_drm.h |   28 ++
  4 files changed, 92 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7902d97..c919832 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1903,6 +1903,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, 
i915_gem_context_create_ioctl, DRM_UNLOCKED),
DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, 
i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_UNLOCKED),
+   DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATUS, 
i915_gem_context_get_reset_status_ioctl, DRM_UNLOCKED),
  };

  int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 69c9856..a4d06f2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1267,3 +1267,64 @@ int i915_reg_read_ioctl(struct drm_device *dev,

return 0;
  }
+
+int i915_gem_context_get_reset_status_ioctl(struct drm_device *dev,
+   void *data, struct drm_file *file)
+{
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   struct intel_ring_buffer *ring;
+   struct drm_i915_reset_status *args = data;
+   struct ctx_reset_state *rs = NULL;
+   unsigned long reset_cnt;
+   u32 reset_status = I915_RESET_UNKNOWN;
+   int ret;
+
+   ret = mutex_lock_interruptible(&dev->struct_mutex);
+   if (ret)
+   return ret;
+
+   ring = &dev_priv->ring[RCS];
+
+   ret = i915_gem_context_get_reset_state(ring,
+  file,
+  args->ctx_id,
+  &rs);
+   if (ret)
+   goto out;
+
+   BUG_ON(!rs);
+
+   reset_cnt = atomic_read(&dev_priv->gpu_error.reset_counter);
+
+   if (reset_cnt & I915_RESET_IN_PROGRESS_FLAG ||


In this case, I believe we're supposed to return the reset state to the 
application.  The ARB_robustness spec says:


"If a reset status other than NO_ERROR is returned and subsequent
calls return NO_ERROR, the context reset was encountered and
completed. If a reset status is repeatedly returned, the context may
be in the process of resetting."

If the reset takes a long time, it seems that even a well-behaved app 
could run afoul of the 'banned' logic.



+   reset_cnt == I915_WEDGED) {
+   goto out;
+   }
+
+   /* Set guilty/innocent status if only one reset was
+* observed and if only one guilty was found
+*/
+   if ((rs->reset_cnt + 2) == reset_cnt &&
+   (rs->guilty_cnt + 1) == dev_priv->gpu_error.guilty_cnt) {


This logic seems... wrong, or at least weird.  "rs->reset_cnt + 2" is 
confusing next to "if only one reset was observed".


dev_priv->gpu_error.reset_counter is the global GPU reset count since 
start-up, and rs->reset_cnt is the global GPU count since start-up when 
the context was created.  Right?


If that's the case, this will cause a context that was completely idle 
(i.e., didn't actually lose anything) to get a reset notification. 
That's an absolute deal breaker.


If that's not the case, then this architecture needs a lot more 
documentation so that people new to it can understand what's happening.



+   reset_status = 0;
+
+   if (rs->guilty)
+   reset_status |= I915_RESET_BATCH_ACTIVE;
+
+   if (rs->innocent)
+   reset_status |= I915_RESET_BATCH_PENDING;
+
+   if (reset_status == 0)
+   reset_status = I915_RESET_UNKNOWN;
+   } else if (rs->reset_cnt == reset_cnt) {
+   reset_status = I915_RESET_NO_ERROR;
+   }
+
+out:
+   if (!ret)
+   args->reset_status = reset_status;
+
+   mutex_unlock(&dev->struct_mutex);
+
+   return ret ? -EINVAL : 0;
+}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3e11acf..2e5e8e7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1712,6 +1712,8 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, 
void *data,
  struct drm_file *file);
  int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
   struct drm_file *file);
+int i915_gem_context_get_reset_status_ioctl(struct drm_device *dev,
+ 

Re: [Intel-gfx] [PATCH v2 10/16] drm/i915: add struct ctx_reset_state

[Ian Romanick]

On 03/14/2013 08:52 AM, Mika Kuoppala wrote:

To count context losses, add struct ctx_reset_state for
both i915_hw_context and drm_i915_file_private.
drm_i915_file_private is used when there is no context.

Signed-off-by: Mika Kuoppala 
---
  drivers/gpu/drm/i915/i915_dma.c |4 +++-
  drivers/gpu/drm/i915/i915_drv.h |   19 +++
  drivers/gpu/drm/i915/i915_gem_context.c |   11 +++
  3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e16099b..7902d97 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1792,7 +1792,7 @@ int i915_driver_open(struct drm_device *dev, struct 
drm_file *file)
struct drm_i915_file_private *file_priv;

DRM_DEBUG_DRIVER("\n");
-   file_priv = kmalloc(sizeof(*file_priv), GFP_KERNEL);
+   file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL);
if (!file_priv)
return -ENOMEM;

@@ -1801,6 +1801,8 @@ int i915_driver_open(struct drm_device *dev, struct 
drm_file *file)
spin_lock_init(&file_priv->mm.lock);
INIT_LIST_HEAD(&file_priv->mm.request_list);

+   i915_gem_context_init_reset_state(dev, &file_priv->reset_state);
+
idr_init(&file_priv->context_idr);

return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a54c507..d004548 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -433,6 +433,19 @@ struct i915_hw_ppgtt {
void (*cleanup)(struct i915_hw_ppgtt *ppgtt);
  };

+struct ctx_reset_state {
+   /* guilty and reset counts when context initialized */
+   unsigned long guilty_cnt;
+   unsigned long reset_cnt;


I think we can afford to spell out "count."  The first time I saw cnt, 
it looked like a dirty word. :)


I think this structure could you some better description of the overall 
architecture.  It's not completely obvious from the individual pieces... 
and that makes it really hard to evaluate.


reset_cnt is the number of resets since start-up.  What is guilty_cnt? 
What are innocent and guilty (below)?


All of this makes it difficult for me to tell whether or not the logic 
in patch 16 is correct... and I don't think it is.



+
+   unsigned innocent;
+   unsigned guilty;
+


/* Time when this context was last blamed for a GPU reset.
 */

+   unsigned long last_guilty_reset;
+
+   /* banned to submit more work */
+   bool banned;
+};

  /* This must match up with the value previously used for execbuf2.rsvd1. */
  #define DEFAULT_CONTEXT_ID 0
@@ -443,6 +456,7 @@ struct i915_hw_context {
struct drm_i915_file_private *file_priv;
struct intel_ring_buffer *ring;
struct drm_i915_gem_object *obj;
+   struct ctx_reset_state reset_state;
  };

  enum no_fbc_reason {
@@ -805,6 +819,7 @@ struct i915_gpu_error {

unsigned long last_reset;

+   unsigned long guilty_cnt;
/**
 * State variable and reset counter controlling the reset flow
 *
@@ -1257,6 +1272,8 @@ struct drm_i915_file_private {
struct list_head request_list;
} mm;
struct idr context_idr;
+
+   struct ctx_reset_state reset_state;
  };

  #define INTEL_INFO(dev)   (((struct drm_i915_private *) 
(dev)->dev_private)->info)
@@ -1677,6 +1694,8 @@ struct i915_hw_context * __must_check
  i915_switch_context(struct intel_ring_buffer *ring,
struct drm_file *file, int to_id);
  void i915_gem_context_free(struct kref *ctx_ref);
+void i915_gem_context_init_reset_state(struct drm_device *dev,
+  struct ctx_reset_state *rs);
  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
  struct drm_file *file);
  int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 8fb4d3c..dbd14b8 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -145,6 +145,15 @@ static void do_destroy(struct i915_hw_context *ctx)
kfree(ctx);
  }

+void i915_gem_context_init_reset_state(struct drm_device *dev,
+  struct ctx_reset_state *rs)
+{
+   struct drm_i915_private *dev_priv = dev->dev_private;
+
+   rs->reset_cnt = atomic_read(&dev_priv->gpu_error.reset_counter);
+   rs->guilty_cnt = dev_priv->gpu_error.guilty_cnt;
+}
+
  static struct i915_hw_context *
  create_hw_context(struct drm_device *dev,
  struct drm_i915_file_private *file_priv)
@@ -177,6 +186,8 @@ create_hw_context(struct drm_device *dev,

ctx->file_priv = file_priv;

+   i915_gem_context_init_reset_state(dev, &ctx->reset_state);
+
  again:
if (idr_pre_get(&file_priv->context_idr, GFP_

Re: [Intel-gfx] [PATCH 09/13] drm/i915: add reset_state for hw_contexts

[Ian Romanick]

On 02/28/2013 03:14 AM, Chris Wilson wrote:

On Wed, Feb 27, 2013 at 05:15:08PM -0800, Ian Romanick wrote:

On 02/27/2013 01:13 AM, Chris Wilson wrote:

On Tue, Feb 26, 2013 at 05:47:12PM -0800, Ian Romanick wrote:

On 02/26/2013 03:05 AM, Mika Kuoppala wrote:

For arb-robustness, every context needs to have it's own
reset state tracking. Default context will be handled in a identical
way as the no-context case in further down in the patch set.
For no-context case, the reset state will be stored in
the file_priv part.

v2: handle default context inside get_reset_state


This isn't the interface we want.  I already sent you the patches
for Mesa, and you seem to have completely ignored them.  Moreover,
this interface provides no mechanism to query for its existence
(other than relying on the kernel version), and no method to
deprecate it.


It's existence, and the existence of any successor, is the only test you
need to check for the interface. Testing the flag field up front also
lets you know if individual flags are known prior to use.


Based on e-mail discussions, I think danvet agrees with me here.

Putting guilty / innocent counting in kernel puts policy decisions
in the kernel that belong with the user space API that implements
them. Putting these choices in the kernel significantly decreases
how "future proof" the interface is.  Since any kernel/user
interface has to be kept forever, this is just asking for
maintenance trouble down the road.


I think you have the policy standpoint reversed.


Can you elaborate?  I've been out of the kernel loop for a long
time, but I always thought policy and mechanism were supposed to be
separated.  In the case of drivers with a user-mode component, the
mechanism was in the kernel (where else could it be?), and policy
decisions were in user-mode.  This is often at odds with keeping the
interfaces between kernel and user thin, so that may be where my
misunderstanding is.


Right, the idea is to keep policy out of the kernel. I disagree that
your suggestion succeeds in doing this.


I was trying to put the mechanism for determining what things happened 
in the kernel and the policy for interpreting what those things mean in 
user-mode.


Ignoring some bugs / misimplementation in my patch (which I try to 
address below), I thought I was doing that.  User-mode gets some 
information about the state it was in when the reset occurred.  It then 
uses that state to decide the value to return to the application for 
glGetGraphicsResetStatusARB.


After reading the bits below where I talk about other problems in the 
kernel code I posted, maybe you can address specific ways my proposal 
fails.  I want any glaring interface deficiencies fixed before any code 
goes in the kernel... I fully understand the pain, on both sides, of 
shipping a kernel interface only to have to replace it in a year.



[snip to details]


There are two requirements in the ARB_robustness extension (and
additional layered extensions) that I believe cause problems with
all of the proposed counting interfaces.

1. GL contexts that are not affected by a reset (i.e., didn't lose
any objects, didn't lose any rendering, etc.) should not observe it.
This is the big one that all the browser vendors want.  If you have
5 tabs open, a reset caused by a malicious WebGL app in one tab
shouldn't, if possible, force them to rebuild all of their state for
all the other tabs.

2. After a GL context observes a reset, it is garbage.  It must be
destroyed and a new one created.  This means we only need to know
that a reset of any variety affected the context.


For me observes implies the ability for the context to inspect global
system state, whereas I think you mean if the context and its associated
state is affected by a reset.


I had thought about explicitly defining what I meant by a couple terms 
in my previous e-mail, but it was already getting quite long.  I guess I 
should have. :(  Here's what I meant:


A. "Affect the context" means the reset causes the context to lose some 
data.  It may be rendering that was queued (so framebuffer data is 
lost), it may be the contents of a buffer, or it may be something else. 
 Either way, the state of some data is not what the application expects 
it to be because of the reset.


B. "Observe the reset" means the GL context gets some data to know that 
the reset happened.  From the application point of view, this means 
glGetGraphicsResetStatusARB returns a value other than GL_NO_ERROR.


What I was trying the describe in point 1 in my previous message (above) 
is that B should occur if and only if A occurs.  Once B occurs, the 
application has to create a new context... recompile all of its shaders, 
reload all of its textures, reload all of its vertex data, etc.  We 
don't want to make apps do that any more often than absolutely necessary.



In addition, I'm concerned about having one GL 

Re: [Intel-gfx] [PATCH 09/13] drm/i915: add reset_state for hw_contexts

[Ian Romanick]

On 02/27/2013 01:13 AM, Chris Wilson wrote:

On Tue, Feb 26, 2013 at 05:47:12PM -0800, Ian Romanick wrote:

On 02/26/2013 03:05 AM, Mika Kuoppala wrote:

For arb-robustness, every context needs to have it's own
reset state tracking. Default context will be handled in a identical
way as the no-context case in further down in the patch set.
For no-context case, the reset state will be stored in
the file_priv part.

v2: handle default context inside get_reset_state


This isn't the interface we want.  I already sent you the patches
for Mesa, and you seem to have completely ignored them.  Moreover,
this interface provides no mechanism to query for its existence
(other than relying on the kernel version), and no method to
deprecate it.


It's existence, and the existence of any successor, is the only test you
need to check for the interface. Testing the flag field up front also
lets you know if individual flags are known prior to use.


Based on e-mail discussions, I think danvet agrees with me here.

Putting guilty / innocent counting in kernel puts policy decisions
in the kernel that belong with the user space API that implements
them. Putting these choices in the kernel significantly decreases
how "future proof" the interface is.  Since any kernel/user
interface has to be kept forever, this is just asking for
maintenance trouble down the road.


I think you have the policy standpoint reversed.


Can you elaborate?  I've been out of the kernel loop for a long time, 
but I always thought policy and mechanism were supposed to be separated. 
 In the case of drivers with a user-mode component, the mechanism was 
in the kernel (where else could it be?), and policy decisions were in 
user-mode.  This is often at odds with keeping the interfaces between 
kernel and user thin, so that may be where my misunderstanding is.



Also, it's difficult (for me) to tell from the rest of the series
whether or not a context that was not affected by a reset (had no
batches in flight at all) can still observe that a reset occurred.


Yes, the context can observe that the global reset increased, its did
not.


See below for a couple reasons why I think this may be a mistake.


 From the GL point of view, once a context has observed a reset, it's
garbage.  Knowing that it saw 1 reset or 43,000,000 resets is the
same.  Since it's a count, GL will have to query the values before
any rendering happens or it won't know whether the value 6 means
there was a reset or not.

The right interface is a set of flags indicating the state the
context was in when it observed a reset.  As this patch series
stands, Mesa will not use this interface.


This interface conforms to your specifications and interface that you
last posted to the list and were revised on list. There are two
substantive differences; there is *less* policy in the kernel than
before, and the breadcrumb of last reset state was overlooked.

In order to make the reset status work in a multi-threaded environment
the kernel exists in, we have to assume that there will be multiple
actors on the reset status, and they will be comparing the state that
they are interested in. The set of flags that GL understands is
deducible from this interface.


We had some discussion on the list about a proposed interface last year. 
 We had a really good discussion, and both you and Daniel provided some 
really valuable feedback.  The biggest piece of feedback that both of 
you provided was to simplify.  I took that advice to heart.  The 
original interface was clunky and over complicated.


If memory serves, that discussion was on the order of 8 to 10 months 
ago.  Quite a lot has happened since then.  The biggest thing that 
happened is I started implementing the interface we discussed in the 
kernel, libdrm, and Mesa.  In that process I found a number of problems 
with even the simplified interface.  Back in September I used this new 
data to simplify the interface even further.  This allowed both the

kernel and the user-mode code to be much less complex.

Right about that same time the whole Mesa team got super busy.  First we 
had OpenGL 3.1, then we had OpenGL ES 3.0.  During that time all of my 
ARB_robustness work languished.  I didn't want to send another round of 
not-fully-baked ideas (or patches) to the list, so they just sat.  And 
sat.  And sat. :(


At FOSDEM earlier this month, I discussed this with Jesse.  I told him 
that there was approximately epsilon probability that I would get back 
to this work.  As a result, I was going to have to throw it over the 
wall to the kernel team.  At no point did he mention that anyone was 
already working on this.


About a week ago Daniel pulled me into a discussion with Mika about the 
ARB_robustness work.  I dug out my old code, paged back in the memory of 
the work from off-line storage, and provided a bunch of feedback to Mika 
and Daniel.  After I sent around my explanation of wh

Re: [Intel-gfx] [PATCH 09/13] drm/i915: add reset_state for hw_contexts

[Ian Romanick]

On 02/26/2013 05:47 PM, Ian Romanick wrote:

On 02/26/2013 03:05 AM, Mika Kuoppala wrote:

For arb-robustness, every context needs to have it's own
reset state tracking. Default context will be handled in a identical
way as the no-context case in further down in the patch set.
For no-context case, the reset state will be stored in
the file_priv part.

v2: handle default context inside get_reset_state


This isn't the interface we want.  I already sent you the patches for
Mesa, and you seem to have completely ignored them.  Moreover, this
interface provides no mechanism to query for its existence (other than
relying on the kernel version), and no method to deprecate it.

Based on e-mail discussions, I think danvet agrees with me here.

Putting guilty / innocent counting in kernel puts policy decisions in
the kernel that belong with the user space API that implements them.
Putting these choices in the kernel significantly decreases how "future
proof" the interface is.  Since any kernel/user interface has to be kept
forever, this is just asking for maintenance trouble down the road.

Also, it's difficult (for me) to tell from the rest of the series
whether or not a context that was not affected by a reset (had no
batches in flight at all) can still observe that a reset occurred.

 From the GL point of view, once a context has observed a reset, it's
garbage.  Knowing that it saw 1 reset or 43,000,000 resets is the same.
  Since it's a count, GL will have to query the values before any
rendering happens or it won't know whether the value 6 means there was a
reset or not.

The right interface is a set of flags indicating the state the context
was in when it observed a reset.  As this patch series stands, Mesa will
not use this interface.


I almost forgot... it seems like there is a lot of code in this series 
to support GPUs where we don't support (either in the existing kernel 
driver or in the hardware at all) hardware contexts.  Is this really 
necessary?  Is anyone going to implement ARB_robustness for 845?  I have 
no intention to do so.  It seems much better to simplify this code to 
only support the GPUs we are actually going to support in user-mode.


If there are no users, the code is untested.  Repeat Keith's mantra: 
Any code that isn't tested is broken. :)



Signed-off-by: Mika Kuoppala 
---
  drivers/gpu/drm/i915/i915_drv.h |4 
  drivers/gpu/drm/i915/i915_gem_context.c |   34
+++
  2 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 1c67fb2..2cc5817 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1684,6 +1684,10 @@ int i915_switch_context(struct
intel_ring_buffer *ring,
  struct drm_file *file, int to_id,
  struct i915_hw_context **ctx);
  void i915_gem_context_free(struct kref *ctx_ref);
+int i915_gem_context_get_reset_state(struct intel_ring_buffer *ring,
+ struct drm_file *file,
+ u32 id,
+ struct ctx_reset_state **rs);
  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
struct drm_file *file);
  int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index cba54fb..1b14a06 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -304,6 +304,40 @@ static int context_idr_cleanup(int id, void *p,
void *data)
  return 0;
  }

+int i915_gem_context_get_reset_state(struct intel_ring_buffer *ring,
+ struct drm_file *file,
+ u32 id,
+ struct ctx_reset_state **rs)
+{
+struct drm_i915_private *dev_priv = ring->dev->dev_private;
+struct drm_i915_file_private *file_priv = file->driver_priv;
+struct i915_hw_context *to;
+
+if (dev_priv->hw_contexts_disabled)
+return -ENOENT;
+
+if (ring->id != RCS)
+return -EINVAL;
+
+if (rs == NULL)
+return -EINVAL;
+
+if (file == NULL)
+return -EINVAL;
+
+if (id == DEFAULT_CONTEXT_ID) {
+*rs = &file_priv->reset_state;
+} else {
+to = i915_gem_context_get(file->driver_priv, id);
+if (to == NULL)
+return -ENOENT;
+
+*rs = &to->reset_state;
+}
+
+return 0;
+}
+
  void i915_gem_context_close(struct drm_device *dev, struct drm_file
*file)
  {
  struct drm_i915_file_private *file_priv = file->driver_priv;





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

Re: [Intel-gfx] [PATCH 09/13] drm/i915: add reset_state for hw_contexts

[Ian Romanick]

On 02/26/2013 03:05 AM, Mika Kuoppala wrote:

For arb-robustness, every context needs to have it's own
reset state tracking. Default context will be handled in a identical
way as the no-context case in further down in the patch set.
For no-context case, the reset state will be stored in
the file_priv part.

v2: handle default context inside get_reset_state


This isn't the interface we want.  I already sent you the patches for 
Mesa, and you seem to have completely ignored them.  Moreover, this 
interface provides no mechanism to query for its existence (other than 
relying on the kernel version), and no method to deprecate it.


Based on e-mail discussions, I think danvet agrees with me here.

Putting guilty / innocent counting in kernel puts policy decisions in 
the kernel that belong with the user space API that implements them. 
Putting these choices in the kernel significantly decreases how "future 
proof" the interface is.  Since any kernel/user interface has to be kept 
forever, this is just asking for maintenance trouble down the road.


Also, it's difficult (for me) to tell from the rest of the series 
whether or not a context that was not affected by a reset (had no 
batches in flight at all) can still observe that a reset occurred.


From the GL point of view, once a context has observed a reset, it's 
garbage.  Knowing that it saw 1 reset or 43,000,000 resets is the same. 
 Since it's a count, GL will have to query the values before any 
rendering happens or it won't know whether the value 6 means there was a 
reset or not.


The right interface is a set of flags indicating the state the context 
was in when it observed a reset.  As this patch series stands, Mesa will 
not use this interface.



Signed-off-by: Mika Kuoppala 
---
  drivers/gpu/drm/i915/i915_drv.h |4 
  drivers/gpu/drm/i915/i915_gem_context.c |   34 +++
  2 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1c67fb2..2cc5817 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1684,6 +1684,10 @@ int i915_switch_context(struct intel_ring_buffer *ring,
struct drm_file *file, int to_id,
struct i915_hw_context **ctx);
  void i915_gem_context_free(struct kref *ctx_ref);
+int i915_gem_context_get_reset_state(struct intel_ring_buffer *ring,
+struct drm_file *file,
+u32 id,
+struct ctx_reset_state **rs);
  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
  struct drm_file *file);
  int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index cba54fb..1b14a06 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -304,6 +304,40 @@ static int context_idr_cleanup(int id, void *p, void *data)
return 0;
  }

+int i915_gem_context_get_reset_state(struct intel_ring_buffer *ring,
+struct drm_file *file,
+u32 id,
+struct ctx_reset_state **rs)
+{
+   struct drm_i915_private *dev_priv = ring->dev->dev_private;
+   struct drm_i915_file_private *file_priv = file->driver_priv;
+   struct i915_hw_context *to;
+
+   if (dev_priv->hw_contexts_disabled)
+   return -ENOENT;
+
+   if (ring->id != RCS)
+   return -EINVAL;
+
+   if (rs == NULL)
+   return -EINVAL;
+
+   if (file == NULL)
+   return -EINVAL;
+
+   if (id == DEFAULT_CONTEXT_ID) {
+   *rs = &file_priv->reset_state;
+   } else {
+   to = i915_gem_context_get(file->driver_priv, id);
+   if (to == NULL)
+   return -ENOENT;
+
+   *rs = &to->reset_state;
+   }
+
+   return 0;
+}
+
  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
  {
struct drm_i915_file_private *file_priv = file->driver_priv;



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

Re: [Intel-gfx] [RFC] GPU reset notification interface

[Ian Romanick]

On 07/18/2012 09:55 AM, Daniel Vetter wrote:

On Wed, Jul 18, 2012 at 09:23:46AM -0700, Ian Romanick wrote:

On 07/18/2012 02:20 AM, Daniel Vetter wrote:

- The "all contexts in a share group need to receive a reset notification"
   wording is irking me a bit because we currently only track all the
   actively used things. So if another context in that share group isn't
   affected (i.e. doesn't even use any of the potentially corrupted bos),
   is the idea that the kernel grows the required tracking, or should
   userspace always ask the reset state for all contexts and do the math
   itself?


There are a couple reasons that all contexts in a share group need
to get the reset notification.  Consider an application with two
contexts, A and B.  Context A is a worker context that does a bunch
of render-to-texture operations, and context B will eventually
consume those textures.  If context A receives a reset, context B,
even if it hasn't done any rendering in five minutes, has lost data.

The kernel should never have any knowledge about GL share groups.
This is where my simplifying assumption (that all contexts in a
share group share an address space and an fd) comes in handy.  If
context A queries the reset status from the kernel first, it can
reach over and poke the reset status of context B (in the gl_context
structure).  Likewise, if context B queries the kernel first, it can
see that another kernel context in its GL context share group got
reset.


- The above essentially links in with how we blame the guilt and figure
   out who's affected. Especially when there's no hw context around, e.g.
   on the blitter or bsd rings, or when an object is used by another
   process and gets corrupted there. Does the spec make any guarantees for
   such a case? Or should we just blame an unknown_reset for all the
   contexts that belong to the fd that submitted the bad batch.


That sounds right.  If we can't assess innocence or guilt, we should
say guilt is unknown.  There are some GL commands that get generate
blits, but I don't think there's anything that can get commands on
the BSD ring.  That's just for media, right?


As an idea for the above two issues, what about the kernel also keeps a
per-fd reset statistics, which will also be returned with this ioctl?
We'd then restrict the meaning of the ctx fields to only mean "and the
context was actually active". Userspace could then wrap all the per-fd
hang reports into reset_unknown for arb_robustness, but I think this would
be a bit more flexible for other userspace.


Ah, right.  So the DDX or libva could detect resets that affect
them. That's reasonable.


struct drm_context_reset_counts {
__u32 ctx_id;

/**
  * Index of the most recent reset where this context was
 * guilty.  Zero if none.
  */
__u32 ctx_guilty;

/**
  * Index of the most recent reset where this context was active,
 * not guilty.  Zero if none.
  */
__u32 ctx_not_guilty;

/**
  * Index of the most recent reset where this context was active,
 * but guilt was unknown. Zero if none.
  */
__u32 ctx_unknown_guilt;

/**
  * Index of the most recent reset where any batchbuffer submitted
 * through this fd was guilty.  Zero if none.
  */
__u32 fd_guilty;

/**
  * Index of the most recent reset where any batchbuffer submitted
 * through this fd was not guilty, but affected.  Zero if none.
  */
__u32 fd_not_guilty;

/**
  * Index of the most recent reset where any batchbuffer submitted
 * through this fd was affected, but no guilt for the hang could
 * be assigned.  Zero if none.
  */
__u32 fd_unknown_guilt;


Since these three fields are per-fd, shouldn't they go in the
proposed drm_reset_counts structure instead?  If we do that, it
might be better to split into two queries.  One for the per-fd
information, and one for the detailed per-context information.  If
we expect the common case to be no-reset, user space could


Oh, I've missed a bit that your ioctl would read out the state for all
contexts on a given fd. My idea was that this is all the data that the
ioctl retuns (and it would only fill out the ctx fields if you spec a
nonzero context). Userspace would then only need to check whether the
context has a new reset number somewhere (= context suffered from a gpu
reset while it was actively used) and then also check whether the fd
suffered from a reset (= something in that gl share group is potentially
corrupted), mapping it to either victim_reset or unknown_reset (depending
upon what exactly the spec wants).


Consider the case where a process has four contexts, A, B, X, and Y.  A 
and B are in a share group, and X and Y are in a share group not with A 
and B. 

Re: [Intel-gfx] [RFC] GPU reset notification interface

[Ian Romanick]

On 07/18/2012 02:20 AM, Daniel Vetter wrote:

On Tue, Jul 17, 2012 at 03:16:19PM -0700, Ian Romanick wrote:

I'm getting ready to implement the reset notification part of
GL_ARB_robustness in the i965 driver.  There are a bunch of quirky
bits of the extension that are causing some grief in designing the
kernel / user interface.  I think I've settled on an interface that
should meet all of the requirements, but I want to bounce it off
people before I start writing code.

Here's the list of requirements.

- Applications poll for reset status.

- Contexts that did not lose data or rendering should not receive a
reset notification.  This isn't strictly a requirement of the spec,
but it seems like a good practice.  Once an application receives a
reset notification for a context, it is supposed to destroy that
context and start over.

- If one context in an OpenGL share group receives a reset
notification, all contexts in that share group must receive a reset
notification.

- All contexts in a single GL share group will have the same fd.
This isn't a requirement so much as a simplifying assumption.  All
contexts in a share group have to be in the same address space, so I
will assume that means they're all controlled by one DRI driver
instance with a single fd.

- The reset notification given to the application should try to
assign guilt.  There are three values possible: unknown guilt,
you're not guilty, or you are guilty.

- If there are multiple resets between polls, the application should
get the "most guilty" answer.  In other words, if there are two
resets and the context was guilty for one and not the other, the
application should get the guilty notification.

- After the application polls the status, the status should revert
to "no reset occurred."

- If the application polls the status and the reset operation is
still in progress, it should continue to get the reset value until
it is "safe" to begin issuing GL commands again.

At some point I'd like to extend this to give slightly finer grained
mechanism so that a context could be told that everything after a
particular GL sync (fence) operation was lost.  This could prevent
some applications from having to destroy and rebuild their context.
This isn't a requirement, but it's an idea that I've been mulling.

Each time a reset occurs, an internal count is incremented.  This
associates a unique integer, starting with 1, with each reset event.
Each context affected by the reset will have the reset event ID
stored in one its three guilt levels.  An ioctl will be provided
that returns the following data for all contexts associated with a
particular fd.

In addition, it will return the index of any reset operation that is
still in progress.

I think this should be sufficient information for user space to meet
all of the requirements.  I had a conversation with Paul and Ken
about this.  Paul was pretty happy with the idea.  Ken felt like too
little policy was in the kernel, and the resulting interface was too
heavy (I'm paraphrasing).


A few things:
- I agree with Chris that reset_in_progress should go, if userspace can
   sneak in and witness a reset event, we have a bug in the kernel. Since
   very recently, we actually have a few bugs less in that area ;-)


I'm operating under the assumption that, from user space's perspective, 
resets are not instantaneous.  If resets are instantaneous, that may 
change things.


I had envisioned two potential uses for reset_in_progress, but I've 
managed to talk myself out of both.



- The "all contexts in a share group need to receive a reset notification"
   wording is irking me a bit because we currently only track all the
   actively used things. So if another context in that share group isn't
   affected (i.e. doesn't even use any of the potentially corrupted bos),
   is the idea that the kernel grows the required tracking, or should
   userspace always ask the reset state for all contexts and do the math
   itself?


There are a couple reasons that all contexts in a share group need to 
get the reset notification.  Consider an application with two contexts, 
A and B.  Context A is a worker context that does a bunch of 
render-to-texture operations, and context B will eventually consume 
those textures.  If context A receives a reset, context B, even if it 
hasn't done any rendering in five minutes, has lost data.


The kernel should never have any knowledge about GL share groups.  This 
is where my simplifying assumption (that all contexts in a share group 
share an address space and an fd) comes in handy.  If context A queries 
the reset status from the kernel first, it can reach over and poke the 
reset status of context B (in the gl_context structure).  Likewise, if 
context B queries the kernel first, it can see that another kernel 
context in its GL context share group got rese

Re: [Intel-gfx] [RFC] GPU reset notification interface

[Ian Romanick]

On 07/17/2012 03:16 PM, Ian Romanick wrote:

I'm getting ready to implement the reset notification part of
GL_ARB_robustness in the i965 driver.  There are a bunch of quirky bits
of the extension that are causing some grief in designing the kernel /
user interface.  I think I've settled on an interface that should meet
all of the requirements, but I want to bounce it off people before I
start writing code.

Here's the list of requirements.

- Applications poll for reset status.

- Contexts that did not lose data or rendering should not receive a
reset notification.  This isn't strictly a requirement of the spec, but
it seems like a good practice.  Once an application receives a reset
notification for a context, it is supposed to destroy that context and
start over.

- If one context in an OpenGL share group receives a reset notification,
all contexts in that share group must receive a reset notification.

- All contexts in a single GL share group will have the same fd.  This
isn't a requirement so much as a simplifying assumption.  All contexts
in a share group have to be in the same address space, so I will assume
that means they're all controlled by one DRI driver instance with a
single fd.

- The reset notification given to the application should try to assign
guilt.  There are three values possible: unknown guilt, you're not
guilty, or you are guilty.

- If there are multiple resets between polls, the application should get
the "most guilty" answer.  In other words, if there are two resets and
the context was guilty for one and not the other, the application should
get the guilty notification.

- After the application polls the status, the status should revert to
"no reset occurred."

- If the application polls the status and the reset operation is still
in progress, it should continue to get the reset value until it is
"safe" to begin issuing GL commands again.

At some point I'd like to extend this to give slightly finer grained
mechanism so that a context could be told that everything after a
particular GL sync (fence) operation was lost.  This could prevent some
applications from having to destroy and rebuild their context.  This
isn't a requirement, but it's an idea that I've been mulling.

Each time a reset occurs, an internal count is incremented.  This
associates a unique integer, starting with 1, with each reset event.
Each context affected by the reset will have the reset event ID stored
in one its three guilt levels.  An ioctl will be provided that returns
the following data for all contexts associated with a particular fd.

In addition, it will return the index of any reset operation that is
still in progress.

I think this should be sufficient information for user space to meet all
of the requirements.  I had a conversation with Paul and Ken about this.
  Paul was pretty happy with the idea.  Ken felt like too little policy
was in the kernel, and the resulting interface was too heavy (I'm
paraphrasing).

struct drm_context_reset_counts {


Some of the Radeon guys on #dri-devel already told me these are the 
wrong prefixes for something that's not a shared DRM interface.  I guess 
drm_i915_gem is the correct prefix?  It's been a long time since my last 
kernel work. :)



 __u32 ctx_id;

 /**
  * Index of the most recent reset where this context was
  * guilty.  Zero if none.
  */
 __u32 guilty;

 /**
  * Index of the most recent reset where this context was
  * not guilty.  Zero if none.
  */
 __u32 not_guilty;

 /**
  * Index of the most recent reset where guilt was unknown.
  * Zero if none.
  */
 __u32 unknown_guilt;
};

struct drm_reset_counts {
 /** Index of the in-progress reset.  Zero if none. */
 unsigned reset_index_in_progress;

 /** Number of contexts. */
 __u32 num_contexts;

 struct drm_context_reset_counts contexts[0];
};
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Intel-gfx] [RFC] GPU reset notification interface

[Ian Romanick]

I'm getting ready to implement the reset notification part of 
GL_ARB_robustness in the i965 driver.  There are a bunch of quirky bits 
of the extension that are causing some grief in designing the kernel / 
user interface.  I think I've settled on an interface that should meet 
all of the requirements, but I want to bounce it off people before I 
start writing code.


Here's the list of requirements.

- Applications poll for reset status.

- Contexts that did not lose data or rendering should not receive a 
reset notification.  This isn't strictly a requirement of the spec, but 
it seems like a good practice.  Once an application receives a reset 
notification for a context, it is supposed to destroy that context and 
start over.


- If one context in an OpenGL share group receives a reset notification, 
all contexts in that share group must receive a reset notification.


- All contexts in a single GL share group will have the same fd.  This 
isn't a requirement so much as a simplifying assumption.  All contexts 
in a share group have to be in the same address space, so I will assume 
that means they're all controlled by one DRI driver instance with a 
single fd.


- The reset notification given to the application should try to assign 
guilt.  There are three values possible: unknown guilt, you're not 
guilty, or you are guilty.


- If there are multiple resets between polls, the application should get 
the "most guilty" answer.  In other words, if there are two resets and 
the context was guilty for one and not the other, the application should 
get the guilty notification.


- After the application polls the status, the status should revert to 
"no reset occurred."


- If the application polls the status and the reset operation is still 
in progress, it should continue to get the reset value until it is 
"safe" to begin issuing GL commands again.


At some point I'd like to extend this to give slightly finer grained 
mechanism so that a context could be told that everything after a 
particular GL sync (fence) operation was lost.  This could prevent some 
applications from having to destroy and rebuild their context.  This 
isn't a requirement, but it's an idea that I've been mulling.


Each time a reset occurs, an internal count is incremented.  This 
associates a unique integer, starting with 1, with each reset event. 
Each context affected by the reset will have the reset event ID stored 
in one its three guilt levels.  An ioctl will be provided that returns 
the following data for all contexts associated with a particular fd.


In addition, it will return the index of any reset operation that is 
still in progress.


I think this should be sufficient information for user space to meet all 
of the requirements.  I had a conversation with Paul and Ken about this. 
 Paul was pretty happy with the idea.  Ken felt like too little policy 
was in the kernel, and the resulting interface was too heavy (I'm 
paraphrasing).


struct drm_context_reset_counts {
__u32 ctx_id;

/**
 * Index of the most recent reset where this context was
 * guilty.  Zero if none.
 */
__u32 guilty;

/**
 * Index of the most recent reset where this context was
 * not guilty.  Zero if none.
 */
__u32 not_guilty;

/**
 * Index of the most recent reset where guilt was unknown.
 * Zero if none.
 */
__u32 unknown_guilt;
};

struct drm_reset_counts {
/** Index of the in-progress reset.  Zero if none. */
unsigned reset_index_in_progress;

/** Number of contexts. */
__u32 num_contexts;

struct drm_context_reset_counts contexts[0];
};
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] gen6 (SNB) depthbuffer issue with OpenGL games

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/20/2011 01:24 AM, Nicolas Kalkhof wrote:
> Hi Ian,
> 
> ok I've definately nailed the issue down to the "i915_enable_rc6" parameter 
> in i915_drv.c
> Someone disabled this parameter and that causes the depthbuffer issue. I've 
> enabled the switch by setting i915_enable_rc6=1; and the problem dissapeared.
> 
> glxinfo | grep "OpenGL renderer" shows:
> OpenGL renderer string: Mesa DRI Intel(R) Sandybridge Mobile
> 
> lspci -vn | grep VGA yields:
> 00:02.0 0300: 8086:0126 (rev 09) (prog-if 00 [VGA controller])
> 
> My CPU is a SNB I72620M with a 3000 graphics. No other GPU present.
> 
> Stupid question: Can I file a bug report on https://bugs.freedesktop.org or 
> on  https://bugzilla.kernel.org/

bugzilla.kernel.org is the right place.  Please include all of the
information from this e-mail and the image showing the corruption.

> -Ursprüngliche Nachricht-
> Von: "Ian Romanick" 
> Gesendet: Jul 19, 2011 11:08:14 PM
> An: "Nicolas Kalkhof" 
> Betreff: Re: [Intel-gfx] gen6 (SNB) depthbuffer issue with OpenGL games
> 
> On 07/19/2011 01:21 PM, Nicolas Kalkhof wrote:
>>>> Hi all,
>>>>
>>>> ok I've nailed the issue down to 3.0.0-rc7 and 3.0.0-rc7-git1. I suspect 
>>>> that the changes made in
>>>> drivers/gpu/drm/i915/i915_dma.c are the cause of the problem.
>>>>
>>>> http://www.kernel.org/diff/diffview.cgi?file=%2Fpub%2Flinux%2Fkernel%2Fv3.0%2Fsnapshots%2Fpatch-3.0-rc7-git1.bz2;z=14
>>>>
>>>> Any Clues?
> 
> Okay, so that's the commit below, which changes some error clean-up
> paths. That is also odd. What *exact* GPU do you have? Specificially,
> what's the output of
> 
> glxinfo | grep "OpenGL renderer"
> 
> and
> 
> lspci -vn | grep VGA
> 
> Does this appear in all games or just certain games? If it's just in
> certain games, which ones?
> 
> commit a7b85d2aa63ed09cd5a4a640772b3272f5ac7caa
> Author: Keith Packard 
> Date: Sun Jul 10 13:12:17 2011 -0700
> 
> drm/i915: Clean up i915_driver_load failure path
> 
> i915_driver_load adds a write-combining MTRR region for the GTT
> aperture to improve memory speeds through the aperture. If
> i915_driver_load fails after this, it would not have cleaned up the
> MTRR. This shouldn't cause any problems, except for consuming an MTRR
> register. Still, it's best to clean up completely in the failure path,
> which is easily done by calling mtrr_del if the mtrr was successfully
> allocated.
> 
> i915_driver_load calls i915_gem_load which register
> i915_gem_inactive_shrink. If i915_driver_load fails after calling
> i915_gem_load, the shrinker will be left registered. When called, it
> will access freed memory and crash. The fix is to unregister the
> shrinker in the
> failure path using code duplicated from i915_driver_unload.
> 
> i915_driver_load also has some incorrect gotos in the error cleanup
> paths:
> 
> * After failing to initialize the GTT (which cannot happen, btw,
> intel_gtt_get returns a fixed (non-NULL) value), it tries to
> free the uninitialized WC IO mapping. Fixed this by changing the
> target from out_iomapfree to out_rmmap
> 
> Signed-off-by: Keith Packard 
> Tested-by: Lin Ming 
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk4m6yAACgkQX1gOwKyEAw/6MQCgjs/YWI3rpwN8XsgHy/rwuq5P
c84AnjsjBjudlE9QZBuLFhuZgW+giw+/
=eJ0i
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] gen6 (SNB) depthbuffer issue with OpenGL games

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/19/2011 01:21 PM, Nicolas Kalkhof wrote:
> Hi all,
> 
> ok I've nailed the issue down to 3.0.0-rc7 and 3.0.0-rc7-git1. I suspect that 
> the changes made in
> drivers/gpu/drm/i915/i915_dma.c are the cause of the problem.
> 
> http://www.kernel.org/diff/diffview.cgi?file=%2Fpub%2Flinux%2Fkernel%2Fv3.0%2Fsnapshots%2Fpatch-3.0-rc7-git1.bz2;z=14
> 
> Any Clues?

Okay, so that's the commit below, which changes some error clean-up
paths.  That is also odd.  What *exact* GPU do you have?  Specificially,
what's the output of

glxinfo | grep "OpenGL renderer"

and

lspci -vn | grep VGA

Does this appear in all games or just certain games?  If it's just in
certain games, which ones?

commit a7b85d2aa63ed09cd5a4a640772b3272f5ac7caa
Author: Keith Packard 
Date:   Sun Jul 10 13:12:17 2011 -0700

drm/i915: Clean up i915_driver_load failure path

i915_driver_load adds a write-combining MTRR region for the GTT
aperture to improve memory speeds through the aperture. If
i915_driver_load fails after this, it would not have cleaned up the
MTRR. This shouldn't cause any problems, except for consuming an MTRR
register. Still, it's best to clean up completely in the failure path,
which is easily done by calling mtrr_del if the mtrr was successfully
allocated.

i915_driver_load calls i915_gem_load which register
i915_gem_inactive_shrink. If i915_driver_load fails after calling
i915_gem_load, the shrinker will be left registered. When called, it
will access freed memory and crash. The fix is to unregister the
shrinker in the
failure path using code duplicated from i915_driver_unload.

i915_driver_load also has some incorrect gotos in the error cleanup
paths:

 * After failing to initialize the GTT (which cannot happen, btw,
   intel_gtt_get returns a fixed (non-NULL) value), it tries to
   free the uninitialized WC IO mapping. Fixed this by changing the
   target from out_iomapfree to out_rmmap

Signed-off-by: Keith Packard 
Tested-by: Lin Ming 
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk4l8j4ACgkQX1gOwKyEAw+XRACaAzuKEihGpktWtA0UbTwx78NR
kBEAoJ7FxkXWEtxC5enkFwnFc19+sPGT
=abq+
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] gen6 (SNB) depthbuffer issue with OpenGL games

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/19/2011 07:58 AM, Nicolas Kalkhof wrote:
> Hi there,
> 
> I've experienced a strange depth buffer issue recently with OpenGL games
> (see attached screenshots). It seems that the depth buffer fails on some
> pixels. This problem was introduced somewhere between
> Kernel-3.0.0-rc6-git6 and kernel-3.0.0-rc7. Both xf-86-video-intel and
> mesa are latest git.
> Could someone please look into it?

That's very odd.  Any chance you could bisect to find the bad commit?
You should be able to just bisect the drivers/gpu/drm directory.  I'm a
bit suspicious that this is a kernel issue.  There were only 5 commits
in drivers/gpu/drm from rc6 to rc7, and none of them should affect any
Intel GPUs at all.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk4l0OEACgkQX1gOwKyEAw/BWQCfeySgLPcT1bdNxPcTJr/S2aw0
vvkAnjS5OJF18pRwrRKaMyG2zoXFqwqB
=AiSG
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 915GME support

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/14/2011 01:52 PM, Terumichi Sadahiro wrote:

> I can't find any description about 915GME (not 915GM) in
> intellinuxgraphics.org at all.
> Does this device have so little demand for, or is there any problem?

There are a lot of different variations of the "same" chip out there.
My guess is that 915GME and 915GM are the same except perhaps clock,
memory support, or CPU support.  Looking at the chipset comparison
(http://ark.intel.com/compare/27715,32279), it seems the primary
difference is the 915GM has TV out and the 915GME does not.  The 915GME
also has "embedded options available," whatever that means.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk4lz6EACgkQX1gOwKyEAw+BrQCgiBtT86rArd3Nj0YlNDr+3j/E
Z+MAn0K/2mw8x3rzaF/m69IQfAGogHpT
=7JZW
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Mesa-dev] [PATCH] intel: Fix stencil buffer to be W tiled

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/18/2011 02:24 PM, Chad Versace wrote:
> On 07/18/2011 02:02 PM, Ian Romanick wrote:
>> On 07/18/2011 01:54 PM, Chad Versace wrote:
>>> On 07/18/2011 11:49 AM, Ian Romanick wrote:
>>>> On 07/18/2011 12:55 AM, Chad Versace wrote:
>>>>> Until now, the stencil buffer was allocated as a Y tiled buffer, because
>>>>> in several locations the PRM states that it is. However, it is actually
>>>>> W tiled. From the PRM, 2011 Sandy Bridge, Volume 1, Part 2, Section
>>>>> 4.5.2.1 W-Major Format:
>>>>> W-Major Tile Format is used for separate stencil.
>>>>
>>>>> The GTT is incapable of W fencing, so we allocate the stencil buffer with
>>>>> I915_TILING_NONE and decode the tile's layout in software.
>>>>
>>>>> This fix touches the following portions of code:
>>>>> - In intel_allocate_renderbuffer_storage(), allocate the stencil
>>>>>   buffer with I915_TILING_NONE.
>>>>> - In intel_verify_dri2_has_hiz(), verify that the stencil buffer is
>>>>>   not tiled.
>>>>> - In the stencil buffer's span functions, the tile's layout must be
>>>>>   decoded in software.
>>>>
>>>>> This commit mutually depends on the xf86-video-intel commit
>>>>> dri: Do not tile stencil buffer
>>>>> Author: Chad Versace 
>>>>> Date:   Mon Jul 18 00:38:00 2011 -0700
>>>>
>>>>> On Gen6 with separate stencil enabled, fixes the following Piglit tests:
>>>>> bugs/fdo23670-drawpix_stencil
>>>>> general/stencil-drawpixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX16-copypixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX16-drawpixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX16-readpixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX1-copypixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX1-drawpixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX1-readpixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX4-copypixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX4-drawpixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX4-readpixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX8-copypixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX8-drawpixels
>>>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX8-readpixels
>>>>> 
>>>>> spec/EXT_packed_depth_stencil/fbo-stencil-GL_DEPTH24_STENCIL8-copypixels
>>>>> 
>>>>> spec/EXT_packed_depth_stencil/fbo-stencil-GL_DEPTH24_STENCIL8-readpixels
>>>>> spec/EXT_packed_depth_stencil/readpixels-24_8
>>>>
>>>>> Note: This is a candidate for the 7.11 branch.
>>>>
>>>>> CC: Eric Anholt 
>>>>> CC: Kenneth Graunke 
>>>>> Signed-off-by: Chad Versace 
>>>>> ---
>>>>>  src/mesa/drivers/dri/intel/intel_clear.c   |6 ++
>>>>>  src/mesa/drivers/dri/intel/intel_context.c |9 ++-
>>>>>  src/mesa/drivers/dri/intel/intel_fbo.c |   13 +++--
>>>>>  src/mesa/drivers/dri/intel/intel_screen.h  |9 ++-
>>>>>  src/mesa/drivers/dri/intel/intel_span.c|   85 
>>>>> 
>>>>>  5 files changed, 89 insertions(+), 33 deletions(-)
>>>>
>>>>> diff --git a/src/mesa/drivers/dri/intel/intel_clear.c 
>>>>> b/src/mesa/drivers/dri/intel/intel_clear.c
>>>>> index dfca03c..5ab9873 100644
>>>>> --- a/src/mesa/drivers/dri/intel/intel_clear.c
>>>>> +++ b/src/mesa/drivers/dri/intel/intel_clear.c
>>>>> @@ -143,6 +143,12 @@ intelClear(struct gl_context *ctx, GLbitfield mask)
>>>>>*/
>>>>>  tri_mask |= BUFFER_BIT_STENCIL;
>>>>>   }
>>>>> +  else if (intel->has_separate_stencil &&
>>>>> +stencilRegion->tiling == I915_TILING_NONE) {
>>>>> + /* The stencil buffer is actually W tiled, which the hardware
>>>>> +  * cannot blit to. */
>>>>> + tri_mask

Re: [Intel-gfx] [Mesa-dev] [PATCH] intel: Fix stencil buffer to be W tiled

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/18/2011 01:54 PM, Chad Versace wrote:
> On 07/18/2011 11:49 AM, Ian Romanick wrote:
>> On 07/18/2011 12:55 AM, Chad Versace wrote:
>>> Until now, the stencil buffer was allocated as a Y tiled buffer, because
>>> in several locations the PRM states that it is. However, it is actually
>>> W tiled. From the PRM, 2011 Sandy Bridge, Volume 1, Part 2, Section
>>> 4.5.2.1 W-Major Format:
>>> W-Major Tile Format is used for separate stencil.
>>
>>> The GTT is incapable of W fencing, so we allocate the stencil buffer with
>>> I915_TILING_NONE and decode the tile's layout in software.
>>
>>> This fix touches the following portions of code:
>>> - In intel_allocate_renderbuffer_storage(), allocate the stencil
>>>   buffer with I915_TILING_NONE.
>>> - In intel_verify_dri2_has_hiz(), verify that the stencil buffer is
>>>   not tiled.
>>> - In the stencil buffer's span functions, the tile's layout must be
>>>   decoded in software.
>>
>>> This commit mutually depends on the xf86-video-intel commit
>>> dri: Do not tile stencil buffer
>>> Author: Chad Versace 
>>> Date:   Mon Jul 18 00:38:00 2011 -0700
>>
>>> On Gen6 with separate stencil enabled, fixes the following Piglit tests:
>>> bugs/fdo23670-drawpix_stencil
>>> general/stencil-drawpixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX16-copypixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX16-drawpixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX16-readpixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX1-copypixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX1-drawpixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX1-readpixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX4-copypixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX4-drawpixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX4-readpixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX8-copypixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX8-drawpixels
>>> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX8-readpixels
>>> spec/EXT_packed_depth_stencil/fbo-stencil-GL_DEPTH24_STENCIL8-copypixels
>>> spec/EXT_packed_depth_stencil/fbo-stencil-GL_DEPTH24_STENCIL8-readpixels
>>> spec/EXT_packed_depth_stencil/readpixels-24_8
>>
>>> Note: This is a candidate for the 7.11 branch.
>>
>>> CC: Eric Anholt 
>>> CC: Kenneth Graunke 
>>> Signed-off-by: Chad Versace 
>>> ---
>>>  src/mesa/drivers/dri/intel/intel_clear.c   |6 ++
>>>  src/mesa/drivers/dri/intel/intel_context.c |9 ++-
>>>  src/mesa/drivers/dri/intel/intel_fbo.c |   13 +++--
>>>  src/mesa/drivers/dri/intel/intel_screen.h  |9 ++-
>>>  src/mesa/drivers/dri/intel/intel_span.c|   85 
>>> 
>>>  5 files changed, 89 insertions(+), 33 deletions(-)
>>
>>> diff --git a/src/mesa/drivers/dri/intel/intel_clear.c 
>>> b/src/mesa/drivers/dri/intel/intel_clear.c
>>> index dfca03c..5ab9873 100644
>>> --- a/src/mesa/drivers/dri/intel/intel_clear.c
>>> +++ b/src/mesa/drivers/dri/intel/intel_clear.c
>>> @@ -143,6 +143,12 @@ intelClear(struct gl_context *ctx, GLbitfield mask)
>>>  */
>>>  tri_mask |= BUFFER_BIT_STENCIL;
>>>   }
>>> +else if (intel->has_separate_stencil &&
>>> +  stencilRegion->tiling == I915_TILING_NONE) {
>>> +   /* The stencil buffer is actually W tiled, which the hardware
>>> +* cannot blit to. */
>>> +   tri_mask |= BUFFER_BIT_STENCIL;
>>> +}
>>>   else {
>>>  /* clearing all stencil bits, use blitting */
>>>  blit_mask |= BUFFER_BIT_STENCIL;
>>> diff --git a/src/mesa/drivers/dri/intel/intel_context.c 
>>> b/src/mesa/drivers/dri/intel/intel_context.c
>>> index 2ba1363..fe8be08 100644
>>> --- a/src/mesa/drivers/dri/intel/intel_context.c
>>> +++ b/src/mesa/drivers/dri/intel/intel_context.c
>>> @@ -1439,7 +1439,12 @@ intel_verify_dri2_has_hiz(struct intel_context 
>>> *intel,
>>>assert(stencil_rb->Base.Format == MESA_FORMAT_S8);
>

Re: [Intel-gfx] [Mesa-dev] [PATCH] dri: Do not tile stencil buffer

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/18/2011 12:28 PM, Chad Versace wrote:
> On 07/18/2011 11:45 AM, Ian Romanick wrote:
>> On 07/18/2011 12:55 AM, Chad Versace wrote:
>>> Until now, the stencil buffer was allocated as a Y tiled buffer, because
>>> in several locations the PRM states that it is. However, it is actually
>>> W tiled. From the PRM, 2011 Sandy Bridge, Volume 1, Part 2, Section
>>> 4.5.2.1 W-Major Format:
>>> W-Major Tile Format is used for separate stencil.
>>
>>> The GTT is incapable of W fencing, so we allocate the stencil buffer with
>>> I915_TILING_NONE and decode the tile's layout in software.
>>
>>> This commit mutually depends on the mesa commit:
>>> intel: Fix stencil buffer to be W tiled
>>> Author: Chad Versace 
>>> Date:   Mon Jul 18 00:37:45 2011 -0700
>>
>>> CC: Eric Anholt 
>>> CC: Kenneth Graunke 
>>> Signed-off-by: Chad Versace 
>>> ---
>>>  src/intel_dri.c |   16 
>>>  1 files changed, 12 insertions(+), 4 deletions(-)
>>
>>> diff --git a/src/intel_dri.c b/src/intel_dri.c
>>> index 5ea7c2c..4652dc7 100644
>>> --- a/src/intel_dri.c
>>> +++ b/src/intel_dri.c
>>> @@ -336,7 +336,6 @@ I830DRI2CreateBuffer(DrawablePtr drawable, unsigned int 
>>> attachment,
>>> switch (attachment) {
>>> case DRI2BufferDepth:
>>> case DRI2BufferDepthStencil:
>>> -   case DRI2BufferStencil:
>>> case DRI2BufferHiz:
>>> if (SUPPORTS_YTILING(intel)) {
>>> hint |= INTEL_CREATE_PIXMAP_TILING_Y;
>>> @@ -351,6 +350,14 @@ I830DRI2CreateBuffer(DrawablePtr drawable, unsigned 
>>> int attachment,
>>> case DRI2BufferFrontRight:
>>> hint |= INTEL_CREATE_PIXMAP_TILING_X;
>>> break;
>>> +   case DRI2BufferStencil:
>>> +   /*
>>> +* The stencil buffer is W tiled. However, we
>>> +* request from the kernel a non-tiled buffer
>>> +* because the GTT is incapable of W fencing.
>>> +*/
>>> +   hint |= INTEL_CREATE_PIXMAP_TILING_NONE;
>>> +   break;
>>> default:
>>> free(privates);
>>> free(buffer);
>>
>> Eh... it seems like this will break compatibility with older versions of
>> Mesa.  I haven't dug around, but there used to be a hack in DRI2 where a
>> client would request a depth buffer and a stencil buffer, but it would
>> get the same packed depth-stencil buffer for both.  I guess that might
>> all be up in the DRI2 layer and not in the driver...
> 
> The 'case DRI2BufferStencil' modified in this hunk was added by me when 
> implementing
> separate stencil support. It was never used for this hack.
> 
> That hack is implemented by an alternate definition of I830DRI2CreateBuffer() 
> which
> is ifdef'd out when DRI2INFOREC_VERSION >= 2. FYI, the line that implements 
> the hack can
> be found by grepping xf86-video-intel:src/intel_dri.c for
> } else if (attachments[i] == DRI2BufferStencil && pDepthPixmap) {

Ah.  That sounds right.

This patch (with the whitespace fix) is

Reviewed-by: Ian Romanick 

>>
>>> @@ -368,11 +375,12 @@ I830DRI2CreateBuffer(DrawablePtr drawable, unsigned 
>>> int attachment,
>>>  * To accomplish this, we resort to the nasty hack of doubling
>>>  * the drm region's cpp and halving its height.
>>>  *
>>> -* If we neglect to double the pitch, then
>>> -* drm_intel_gem_bo_map_gtt() maps the memory incorrectly.
>>> +* If we neglect to double the pitch, then render corruption
>>> + * occurs.
>>
>> Mangled whitespace?  Probably mixed tabs and spaces...
> 
> Oops. Will fix.
> 
>>
>>>  */
>>> if (attachment == DRI2BufferStencil) {
>>> -   pixmap_height /= 2;
>>> +   pixmap_width = ALIGN(pixmap_width, 64);
>>> +   pixmap_height = ALIGN(pixmap_height / 2, 64);
>>> pixmap_cpp *= 2;
>>> }
> 
> 

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk4knooACgkQX1gOwKyEAw8mrACdFEbKAyQIntzWec6LVA2ZYYjz
8icAn1Fc+vHRyNeygch2QHnH7Vh9Ilqt
=9yZk
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Mesa-dev] [PATCH] intel: Fix stencil buffer to be W tiled

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/18/2011 12:55 AM, Chad Versace wrote:
> Until now, the stencil buffer was allocated as a Y tiled buffer, because
> in several locations the PRM states that it is. However, it is actually
> W tiled. From the PRM, 2011 Sandy Bridge, Volume 1, Part 2, Section
> 4.5.2.1 W-Major Format:
> W-Major Tile Format is used for separate stencil.
> 
> The GTT is incapable of W fencing, so we allocate the stencil buffer with
> I915_TILING_NONE and decode the tile's layout in software.
> 
> This fix touches the following portions of code:
> - In intel_allocate_renderbuffer_storage(), allocate the stencil
>   buffer with I915_TILING_NONE.
> - In intel_verify_dri2_has_hiz(), verify that the stencil buffer is
>   not tiled.
> - In the stencil buffer's span functions, the tile's layout must be
>   decoded in software.
> 
> This commit mutually depends on the xf86-video-intel commit
> dri: Do not tile stencil buffer
> Author: Chad Versace 
> Date:   Mon Jul 18 00:38:00 2011 -0700
> 
> On Gen6 with separate stencil enabled, fixes the following Piglit tests:
> bugs/fdo23670-drawpix_stencil
> general/stencil-drawpixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX16-copypixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX16-drawpixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX16-readpixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX1-copypixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX1-drawpixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX1-readpixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX4-copypixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX4-drawpixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX4-readpixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX8-copypixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX8-drawpixels
> spec/EXT_framebuffer_object/fbo-stencil-GL_STENCIL_INDEX8-readpixels
> spec/EXT_packed_depth_stencil/fbo-stencil-GL_DEPTH24_STENCIL8-copypixels
> spec/EXT_packed_depth_stencil/fbo-stencil-GL_DEPTH24_STENCIL8-readpixels
> spec/EXT_packed_depth_stencil/readpixels-24_8
> 
> Note: This is a candidate for the 7.11 branch.
> 
> CC: Eric Anholt 
> CC: Kenneth Graunke 
> Signed-off-by: Chad Versace 
> ---
>  src/mesa/drivers/dri/intel/intel_clear.c   |6 ++
>  src/mesa/drivers/dri/intel/intel_context.c |9 ++-
>  src/mesa/drivers/dri/intel/intel_fbo.c |   13 +++--
>  src/mesa/drivers/dri/intel/intel_screen.h  |9 ++-
>  src/mesa/drivers/dri/intel/intel_span.c|   85 
> 
>  5 files changed, 89 insertions(+), 33 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/intel/intel_clear.c 
> b/src/mesa/drivers/dri/intel/intel_clear.c
> index dfca03c..5ab9873 100644
> --- a/src/mesa/drivers/dri/intel/intel_clear.c
> +++ b/src/mesa/drivers/dri/intel/intel_clear.c
> @@ -143,6 +143,12 @@ intelClear(struct gl_context *ctx, GLbitfield mask)
>*/
>  tri_mask |= BUFFER_BIT_STENCIL;
>   }
> +  else if (intel->has_separate_stencil &&
> +stencilRegion->tiling == I915_TILING_NONE) {
> + /* The stencil buffer is actually W tiled, which the hardware
> +  * cannot blit to. */
> + tri_mask |= BUFFER_BIT_STENCIL;
> +  }
>   else {
>  /* clearing all stencil bits, use blitting */
>  blit_mask |= BUFFER_BIT_STENCIL;
> diff --git a/src/mesa/drivers/dri/intel/intel_context.c 
> b/src/mesa/drivers/dri/intel/intel_context.c
> index 2ba1363..fe8be08 100644
> --- a/src/mesa/drivers/dri/intel/intel_context.c
> +++ b/src/mesa/drivers/dri/intel/intel_context.c
> @@ -1439,7 +1439,12 @@ intel_verify_dri2_has_hiz(struct intel_context *intel,
>assert(stencil_rb->Base.Format == MESA_FORMAT_S8);
>assert(depth_rb && depth_rb->Base.Format == MESA_FORMAT_X8_Z24);
>  
> -  if (stencil_rb->region->tiling == I915_TILING_Y) {
> +  if (stencil_rb->region->tiling == I915_TILING_NONE) {
> +  /*
> +   * The stencil buffer is actually W tiled. The region's tiling is
> +   * I915_TILING_NONE, however, because the GTT is incapable of W
> +   * fencing.
> +   */
>intel->intelScreen->dri2_has_hiz = INTEL_DRI2_HAS_HIZ_TRUE;
>return;
>} else {
> @@ -1527,7 +1532,7 @@ intel_verify_dri2_has_hiz(struct intel_context *intel,
> * Presently, however, no verification or clean up is necessary, and
> * execution should not reach here. If the framebuffer still has a hiz
> * region, then we have already set dri2_has_hiz to true after
> -   * confirming above that the stencil buffer is Y tiled.
> +   * confirming above that the stencil buffer is W tiled.
> */
>assert(0);
> }
> diff --git a/src/mesa/

Re: [Intel-gfx] [Mesa-dev] [PATCH] dri: Do not tile stencil buffer

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/18/2011 12:55 AM, Chad Versace wrote:
> Until now, the stencil buffer was allocated as a Y tiled buffer, because
> in several locations the PRM states that it is. However, it is actually
> W tiled. From the PRM, 2011 Sandy Bridge, Volume 1, Part 2, Section
> 4.5.2.1 W-Major Format:
> W-Major Tile Format is used for separate stencil.
> 
> The GTT is incapable of W fencing, so we allocate the stencil buffer with
> I915_TILING_NONE and decode the tile's layout in software.
> 
> This commit mutually depends on the mesa commit:
> intel: Fix stencil buffer to be W tiled
> Author: Chad Versace 
> Date:   Mon Jul 18 00:37:45 2011 -0700
> 
> CC: Eric Anholt 
> CC: Kenneth Graunke 
> Signed-off-by: Chad Versace 
> ---
>  src/intel_dri.c |   16 
>  1 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/src/intel_dri.c b/src/intel_dri.c
> index 5ea7c2c..4652dc7 100644
> --- a/src/intel_dri.c
> +++ b/src/intel_dri.c
> @@ -336,7 +336,6 @@ I830DRI2CreateBuffer(DrawablePtr drawable, unsigned int 
> attachment,
>   switch (attachment) {
>   case DRI2BufferDepth:
>   case DRI2BufferDepthStencil:
> - case DRI2BufferStencil:
>   case DRI2BufferHiz:
>   if (SUPPORTS_YTILING(intel)) {
>   hint |= INTEL_CREATE_PIXMAP_TILING_Y;
> @@ -351,6 +350,14 @@ I830DRI2CreateBuffer(DrawablePtr drawable, unsigned int 
> attachment,
>   case DRI2BufferFrontRight:
>   hint |= INTEL_CREATE_PIXMAP_TILING_X;
>   break;
> + case DRI2BufferStencil:
> + /*
> +  * The stencil buffer is W tiled. However, we
> +  * request from the kernel a non-tiled buffer
> +  * because the GTT is incapable of W fencing.
> +  */
> + hint |= INTEL_CREATE_PIXMAP_TILING_NONE;
> + break;
>   default:
>   free(privates);
>   free(buffer);

Eh... it seems like this will break compatibility with older versions of
Mesa.  I haven't dug around, but there used to be a hack in DRI2 where a
client would request a depth buffer and a stencil buffer, but it would
get the same packed depth-stencil buffer for both.  I guess that might
all be up in the DRI2 layer and not in the driver...

> @@ -368,11 +375,12 @@ I830DRI2CreateBuffer(DrawablePtr drawable, unsigned int 
> attachment,
>* To accomplish this, we resort to the nasty hack of doubling
>* the drm region's cpp and halving its height.
>*
> -  * If we neglect to double the pitch, then
> -  * drm_intel_gem_bo_map_gtt() maps the memory incorrectly.
> +  * If we neglect to double the pitch, then render corruption
> + * occurs.

Mangled whitespace?  Probably mixed tabs and spaces...

>*/
>   if (attachment == DRI2BufferStencil) {
> - pixmap_height /= 2;
> + pixmap_width = ALIGN(pixmap_width, 64);
> + pixmap_height = ALIGN(pixmap_height / 2, 64);
>   pixmap_cpp *= 2;
>   }
>  
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk4kf10ACgkQX1gOwKyEAw9jVACeOfy5BjD4Nb/YN3vyOoR5MOOv
IOAAn20Lh7GAN7KSAkBFs/u5uaHq8/Sm
=ICWC
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: initialize gen6 rps work queue for Ironlake+

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 05/18/2011 06:21 PM, Jesse Barnes wrote:
> Looks like I didn't merge Ben's RPS work queue stuff correctly with the
> new IRQ split code (diff was sparse enough that git didn't complain).
> This should prevent null derefs on ILK+ due to the missing work queue.

Is this actually needed on Ironlake (i.e., gen5)?

> Signed-off-by:  Jesse Barnes 
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 349a03e..9112bf5 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1736,6 +1736,7 @@ void ironlake_irq_preinstall(struct drm_device *dev)
>  
>   INIT_WORK(&dev_priv->hotplug_work, i915_hotplug_work_func);
>   INIT_WORK(&dev_priv->error_work, i915_error_work_func);
> + INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
>  
>   I915_WRITE(HWSTAM, 0xeffe);
>  
> @@ -1887,7 +1888,6 @@ void i915_driver_irq_preinstall(struct drm_device * dev)
>  
>   INIT_WORK(&dev_priv->hotplug_work, i915_hotplug_work_func);
>   INIT_WORK(&dev_priv->error_work, i915_error_work_func);
> - INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
>  
>   if (I915_HAS_HOTPLUG(dev)) {
>   I915_WRITE(PORT_HOTPLUG_EN, 0);

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk3T9dwACgkQX1gOwKyEAw/wRACeLPeHTmjvu0ObCV3iJsZMXNud
gwUAn3vuXo6J59viQrw5aMn6r8of68pL
=zjHv
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Trying to understand the URB code

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 04/07/2011 01:24 AM, Zou, Nanhai wrote:
> Hi Ken,
>   URB allocation on gen6 is different than previous gens.
> On previous gen, there is a total size urb for many stages of VS GS CLIP SF.
> So driver has to decide how much urb to allocate for each stage.
> 
> On gen6 only GS and VS will share an urb of 64k(32k on GT1)
> 
> urb_entry_number also has a limit, check 3DSTATE_URB command.

Ken and I did some research on this yesterday.  The bspec says the valid
range for the VS is [24, 256] on GT2 and [24, 128] on GT1.  For the GS
it says the valid range is [0, 256] on GT2 and [0, 254] on GT1.  I can't
believe that 254 is correct.

> The more urb entry number we have, the more current threads can spawn.
> 
> But 
> We should make sure 
> VS urb size = single_urb_size * gs_urb_entry 
> +
> GS urb size = single_urb_size * vs_urb_entry 
> < total_urb_size 
> 
> In most case, this is ok, unless we have a huge single_urb_size.
> 
> Since we are always using GS pass through mode for now.
> The number for GS urb dose not matter.
> So allocate more urb size for VS, less for GS may be better for huge urb_size
> 
> But consider we will support GS shader later and huge urb_size case is very 
> rare,
> I separate urb equally for GS and VS.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2fXAYACgkQX1gOwKyEAw+RSQCdFiIxdyeKmytlovCEO3w3RIIA
uN4An1POHfJ3J4fzKZnGWbHLb4eZBwnM
=xpu8
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] [WIP] i965: Use up to 80 WM threads on GT2.

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 03/28/2011 10:55 AM, Eric Anholt wrote:

> Here's an idea for an SNB performance improvement from the specs.  It
> says that on GT2 you should be able to use 80 threads if "WIZ Hashing
> Disable in GT_MODE register enabled".  On my system (supposedly GT2),
> that bit (bit 6 of 0x20d0) is unset.  In testing, with intel_reg_write
> 0x20d0 0x00400040 (it only successfully took once, I suspect due to
> FORCEWAKE, which also means that I can't necessarily trust that the
> bit was unset originally), I got only hangs from 3D.

So, we're currently using too many threads in some cases?  Could this be
related to bug #35730?  In that case the failure seems to be limited to
SugarBay.  I believe that's GT1, but I can never get the code names for
these chips straight.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2RCisACgkQX1gOwKyEAw8nRgCgnC1HMyfVwx5MLiaVF4mVzprQ
oD4An2DMFJdXfYKOyvehR7x6qAQnYix6
=4m/t
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] OpenGL ES 1.1 On GM855

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 11/29/2010 06:06 AM, Clurado cl wrote:
> Hi all , after a month of discussing , debugging and ... i still have
> problem with OpenGL ES1 on Intel GM855 Hardware . i confiused about GL
> ES 1 requirements , so please let me know , is GM855 support OpenGLES1
> Over DRI ?!

It should be *possible* to support ES 1.x on 8xx hardware, but I don't
think anyone has ever done any work on it.  As far as I'm aware, ES 1.x
is, in terms of required hardware support, a subset of OpenGL 1.3, and
that should be supported on 8xx.  We don't do much to support 8xx
anymore, and we don't care much about ES 1.x.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzz+YUACgkQX1gOwKyEAw9ArwCeOi5ROzF8DpDTtqjwhWOMw7qL
M0YAn2ZoOlV6odaMo0T7EUtnG/p9yPd8
=s8XN
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] piglit regression on sandybridge

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 11/29/2010 12:52 AM, Zhenyu Wang wrote:

> Looks the culprit for recent piglit regression on sandybridge
> that caused hang is bisected to

Could you be more specific about the regression?  The commit message
mentions two tests that regress with this change.  Are there other
regressions?  If yes, what are they?

> commit 9effc1adf1e7ba57fb3b10909762b76c1ae12f61
> Author: Eric Anholt 
> Date:   Mon Oct 11 16:02:08 2010 -0700
> 
> i965: re-enable gen6 IF statements in the fragment shader.
> 
> IF statements were getting flattened while they were broken.  With
> Zhenyu's last fix for ENDIF's type, everything appears to have lined
> up to actually work.
> 
> This regresses two tests:
> glsl1-! (not) operator (1, fail)
> glsl1-! (not) operator (1, pass)
> 
> but fixes tests that couldn't work before because the IFs couldn't
> be flattened:
> glsl-fs-discard-01
> occlusion-query-discard
> 
> (and, naturally, this should be a performance improvement for apps
>  that actually use IF statements to avoid executing a bunch of code).
> 
> What may go wrong with this? QA is stalled on this to continue on
> running piglit..

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzz+L8ACgkQX1gOwKyEAw9OMwCgmWz6YYEkX0glsmf4wSngfi1x
dFYAn0F0d5AzQJCxhlBHTjZb/U+CYkC4
=Qt8Y
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Mesa3d-dev] mesa doesn't work with compiz (i965 + tips of all branches)

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Note: I'm sending this reply to mesa-...@lists.freedesktop.org instead
of the old mailing list.

Maxim Levitsky wrote:
> On Tue, 2010-06-29 at 15:49 -0700, Ian Romanick wrote:
> Corbin Simpson wrote:
>>>> Curious. Admittedly I can't look at the content of that commit, but they
>>>> can't be too useless if compiz selects them. IIRC the point was to limit
>>>> the runtime of Intel internal tests; can't those tests be amended
>>>> instead? The number of configs will only grow; r300g has over 200 now
>>>> thanks to multisampling.
> The configs are useless.  Applications can only ask for "bits >= X".
> There are still 24-bit depth / 8-bit stencil configs, and, last time I
> checked, 8 >= 0.  There is no way to ask for a 24/0 config that wouldn't
> instead give a 24/8 config.
> 
>>>> Posting from a mobile, pardon my terseness. ~ C.
>>>>
>>>>> On Jun 29, 2010 1:28 PM, "Maxim Levitsky" >>>> <mailto:maximlevit...@gmail.com>> wrote:
>>>>>
>>>>> On Tue, 2010-06-29 at 20:34 +0300, Maxim Levitsky wrote:
>>>>>> On Sun, 2010-06-27 at 19:07 +0300, Maxim ...
>>>>> Bisected this to
>>>>>
>>>>> 73e24cd5a7a0760726a681dda5b88805ddcf1555 is first bad commit
>>>>> commit 73e24cd5a7a0760726a681dda5b88805ddcf1555
>>>>> Author: Ian Romanick >>>> <mailto:ian.d.roman...@intel.com>>
>>>>> Date:   Mon Feb 8 10:34:52 2010 -0800
>>>>>
>>>>>intel: Stop exposing useless 24 depth/0 stencil configs
> I need two pieces of information:
> 
>   - A diff of the output of glxinfo immediately before and immediately
> after this commit.
> 
>   - A list of what config attributes compiz is requesting.  It should
> be easy enough to instrument choose_visual in glxcmds.c to dump out
> attribList.
> 
> It should be pretty easy to root-cause this problem with that data.

[snip]

> What is interesting is this:
>
> -0x62 32 tc  0 32  0 r  y  .  8  8  8  8  0 24  0  0  0  0  0  0 0 None

Yup.  That has to be it.  The fix will have two parts.  First, make the
3D driver a this specific visual.  That will make "new" 3D drivers work
with "old" 2D drivers.  Second, make the 2D driver mark this visual has
having stencil.  The memory is there (interleaved with the 24-bits of
depth), so we may as well expose it.

If there's not a bugzilla for this, could you create one?  This will
ensure that I get this fixed sooner rather than later.  This also helps
us get it into a stable Mesa release.

Thanks.

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkws0TkACgkQX1gOwKyEAw8AiQCfeBiSF8kwHw5r93HREUpNdFu9
KOIAnRAY2rJUIvrrtYAZ/OAEI4GKmSqY
=bV5i
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Mesa3d-dev] mesa doesn't work with compiz (i965 + tips of all branches)

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Corbin Simpson wrote:
> Curious. Admittedly I can't look at the content of that commit, but they
> can't be too useless if compiz selects them. IIRC the point was to limit
> the runtime of Intel internal tests; can't those tests be amended
> instead? The number of configs will only grow; r300g has over 200 now
> thanks to multisampling.

The configs are useless.  Applications can only ask for "bits >= X".
There are still 24-bit depth / 8-bit stencil configs, and, last time I
checked, 8 >= 0.  There is no way to ask for a 24/0 config that wouldn't
instead give a 24/8 config.

> Posting from a mobile, pardon my terseness. ~ C.
> 
>> On Jun 29, 2010 1:28 PM, "Maxim Levitsky" > <mailto:maximlevit...@gmail.com>> wrote:
>>
>> On Tue, 2010-06-29 at 20:34 +0300, Maxim Levitsky wrote:
>> > On Sun, 2010-06-27 at 19:07 +0300, Maxim ...
>>
>> Bisected this to
>>
>> 73e24cd5a7a0760726a681dda5b88805ddcf1555 is first bad commit
>> commit 73e24cd5a7a0760726a681dda5b88805ddcf1555
>> Author: Ian Romanick > <mailto:ian.d.roman...@intel.com>>
>> Date:   Mon Feb 8 10:34:52 2010 -0800
>>
>>intel: Stop exposing useless 24 depth/0 stencil configs

I need two pieces of information:

  - A diff of the output of glxinfo immediately before and immediately
after this commit.

  - A list of what config attributes compiz is requesting.  It should
be easy enough to instrument choose_visual in glxcmds.c to dump out
attribList.

It should be pretty easy to root-cause this problem with that data.

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwqeGwACgkQX1gOwKyEAw9MfQCZAUWS6wXUjRWYaX++6YRjl4Pk
XMsAn06cgcjJf/dDMCBDTr/tdaFoBsGM
=rGTt
-END PGP SIGNATURE-
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx