[Mesa-dev] [PATCH 39/53] i965/drm: Make register write check handle execbuffer directly.

[Chris Wilson]

On Tue, Apr 04, 2017 at 05:10:29PM -0700, Kenneth Graunke wrote:
> I'm about to rewrite how relocation handling works, at which point
> drm_bacon_bo_emit_reloc() and drm_bacon_bo_mrb_exec() won't exist
> anymore.  This code is already largely not using the batchbuffer
> infrastructure, so just go all the way and handle relocations, the
> validation list, and execbuffer ourselves.  That way, we don't have
> to think the weird case where we only have a screen, and no context,
> when redesigning the relocation handling.
> ---
>  src/mesa/drivers/dri/i965/intel_screen.c | 38 ++++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> index 4cecd52f36a..42baadf25e1 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -1367,6 +1367,9 @@ static bool
>  intel_detect_pipelined_register(struct intel_screen *screen,
>                                  int reg, uint32_t expected_value, bool reset)
>  {
> +   if (screen->no_hw)
> +      return false;
> +
>     drm_bacon_bo *results, *bo;
>     uint32_t *batch;
>     uint32_t offset = 0;
> @@ -1394,11 +1397,14 @@ intel_detect_pipelined_register(struct intel_screen *screen,
>     /* Save the register's value back to the buffer. */
>     *batch++ = MI_STORE_REGISTER_MEM | (3 - 2);
>     *batch++ = reg;
> -   drm_bacon_bo_emit_reloc(bo, (char *)batch -(char *)bo->virtual,
> -                           results, offset*sizeof(uint32_t),
> -                           I915_GEM_DOMAIN_INSTRUCTION,
> -                           I915_GEM_DOMAIN_INSTRUCTION);
> -   *batch++ = ((uint32_t) results->offset64) + offset*sizeof(uint32_t);
> +   struct drm_i915_gem_relocation_entry reloc = {
> +      .offset = (char *) batch - (char *) bo->virtual,
> +      .delta = offset * sizeof(uint32_t),
> +      .target_handle = results->handle,
> +      .read_domains = I915_GEM_DOMAIN_INSTRUCTION,
> +      .write_domain = I915_GEM_DOMAIN_INSTRUCTION,
> +   };
> +   *batch++ = offset * sizeof(uint32_t);

Better as *batch++ = reloc.presumed_offset + reloc.delta;
gcc should reduce that to the reloc.delta already in the register, and
it demonstrates the execbuf rules more clearly.

>     /* And afterwards clear the register */
>     if (reset) {
> @@ -1409,8 +1415,26 @@ intel_detect_pipelined_register(struct intel_screen *screen,
>  
>     *batch++ = MI_BATCH_BUFFER_END;
>  
> -   drm_bacon_bo_mrb_exec(bo, ALIGN((char *)batch - (char *)bo->virtual, 8),
> -                         I915_EXEC_RENDER);
> +   struct drm_i915_gem_exec_object2 exec_objects[2] = {
> +      {
> +         .handle = results->handle,
> +      },
> +      {
> +         .handle = bo->handle,
> +         .relocation_count = 1,
> +         .relocs_ptr = (uintptr_t) &reloc,
> +      }
> +   };
> +
> +   struct drm_i915_gem_execbuffer2 execbuf = {
> +      .buffers_ptr = (uintptr_t) exec_objects,
> +      .buffer_count = 2,
> +      .batch_len = ALIGN((char *) batch - (char *) bo->virtual, 8),
> +      .flags = I915_EXEC_RENDER,
> +   };
> +
> +   __DRIscreen *dri_screen = screen->driScrnPriv;
> +   drmIoctl(dri_screen->fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf);
>  
>     /* Check whether the value got written. */
>     if (drm_bacon_bo_map(results, false) == 0) {

And all the error handling falls out in the wash by simplying disabling
access.

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre