Menu
▾
▴
Re: Initial 915 superioctl patch.
|
From: Jesse B. <jes...@in...> - 2007-10-05 18:24:56
|
On Thursday, October 4, 2007 8:55 pm Dave Airlie wrote:
>
Overall, looks nice.
At a high level, I'm wondering if something like this could be made more
generic... It seems like other GPUs will need similar relocation processing
so maybe the DRM should grow some generic reloc processing code? Much of
this stuff seems fairly generic, so maybe it wouldn't be that hard, and it
would keep us from adding yet another driver specific ioctl...
> +int i915_apply_reloc(struct drm_file *file_priv, int num_buffers,
> + struct drm_buffer_object **buffers,
> + struct i915_relocatee_info *relocatee,
> + uint32_t *reloc)
> +{
> + unsigned index;
> + unsigned long new_cmd_offset;
> + u32 val;
> + int ret;
> +
> + if (reloc[2] >= num_buffers) {
> + DRM_ERROR("Illegal relocation buffer %08X\n", reloc[2]);
> + return -EINVAL;
> + }
> +
> + new_cmd_offset = reloc[0];
> + if (!relocatee->data_page ||
> + !drm_bo_same_page(relocatee->offset, new_cmd_offset)) {
> + drm_bo_kunmap(&relocatee->kmap);
> + relocatee->offset = new_cmd_offset;
> + ret = drm_bo_kmap(relocatee->buf, new_cmd_offset >> PAGE_SHIFT,
> + 1, &relocatee->kmap);
> + if (ret) {
> + DRM_ERROR("Could not map command buffer to apply relocs\n %08x", new_cmd_offset);
> + return ret;
> + }
> +
> + relocatee->data_page = drm_bmo_virtual(&relocatee->kmap,
> + &relocatee->is_iomem);
> + relocatee->page_offset = (relocatee->offset & PAGE_MASK);
> + }
> +
> + val = buffers[reloc[2]]->offset;
> + index = (reloc[0] - relocatee->page_offset) >> 2;
> +
> + /* add in validate */
> + val = val + reloc[1];
> +
> + relocatee->data_page[index] = val;
> + return 0;
> +}
> +
> +#define RELOC_START_OFFSET 2
> +#define RELOC_NUM_INTS 3
These seem unused?
> +int i915_process_relocs(struct drm_file *file_priv,
> + uint32_t buf_handle,
> + uint32_t *reloc_buf_handle,
> + struct i915_relocatee_info *relocatee,
> + struct drm_buffer_object **buffers,
> + uint32_t num_buffers)
> +{
> + struct drm_device *dev = file_priv->head->dev;
> + struct drm_buffer_object *reloc_list_object;
> + int ret;
> + uint32_t cur_handle = *reloc_buf_handle;
> + uint32_t num_relocs;
> + struct drm_bo_kmap_obj reloc_kmap;
> + uint32_t *reloc_page;
> + int reloc_is_iomem;
> + uint32_t reloc_offset, reloc_end, reloc_page_offset, next_offset;
> + int reloc_stride;
> + uint32_t cur_offset;
gcc probably takes care of this, but it's still nice to order your automatic
variables from large to small just to make sure you get good alignment.
> +
> + memset(&reloc_kmap, 0, sizeof(reloc_kmap));
> +
> + reloc_list_object = drm_lookup_buffer_object(file_priv, cur_handle, 1);
> + if (!reloc_list_object)
> + return -EINVAL;
> +
> + ret = drm_bo_kmap(reloc_list_object, 0, 1, &reloc_kmap);
> + if (ret) {
> + DRM_ERROR("Could not map relocation buffer.\n");
> + goto out;
> + }
> +
> + reloc_page = drm_bmo_virtual(&reloc_kmap, &reloc_is_iomem);
> + num_relocs = reloc_page[0] & 0xffff;
> +
> + if ((reloc_page[0] >> 16) & 0xffff) {
> + DRM_ERROR("Unsupported relocation type requested\n");
> + goto out;
> + }
> +
> + /* get next relocate buffer handle */
> + *reloc_buf_handle = reloc_page[1];
> + reloc_stride = 4; /* may be different for other types of relocs */
> +
> + DRM_DEBUG("num relocs is %d, next is %08X\n", num_relocs, reloc_page[1]);
> +
> + reloc_page_offset = 0;
> + reloc_offset = 4 * sizeof(uint32_t);
> + reloc_end = reloc_offset + (num_relocs * reloc_stride * sizeof(uint32_t));
So the first 32 bits of the reloc_page contains the reloc count and type, and
the next bits contain the actual info required, right? It might be nice to cast
them into reloc_info and reloc_arg structures of some kind. That way if new
reloc types were added later things would be a little clearer (and this code
would be a little more straightforward I think too). Would that make sense?
> +static int i915_execbuffer(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> + drm_i915_sarea_t *sarea_priv = (drm_i915_sarea_t *)
> + dev_priv->sarea_priv;
> + struct drm_i915_execbuffer *exec_buf = data;
> + struct _drm_i915_batchbuffer *batch = &exec_buf->batch;
> + struct drm_fence_arg *fence_arg = &exec_buf->fence_arg;
> + int num_buffers;
> + int ret;
> + struct drm_fence_object *fence;
Should data be __user? Oh no, I see it's already been copied in
drm_ioctl()...
> + if (!dev_priv->allow_batchbuffer) {
> + DRM_ERROR("Batchbuffer ioctl disabled\n");
> + return -EINVAL;
> + }
Why would userspace turn this off? Looks like current code turns the feature
on at least...
> +
> +
Nit: extra newline.
> + LOCK_TEST_WITH_RETURN(dev, file_priv);
> +
> + if (batch->num_cliprects && DRM_VERIFYAREA_READ(batch->cliprects,
> + batch->num_cliprects *
> + sizeof(struct drm_clip_rect)))
> + return -EFAULT;
> + num_buffers = I915_NUM_VALIDATE_BUFFERS;
> +
> + /* validate buffer list + fixup relocations */
> + ret = i915_validate_buffer_list(file_priv, 0, exec_buf->ops_list, dev_priv->buffers, &num_buffers);
> + if (ret)
> + return ret;
> +
> + /* submit buffer */
> + batch->start = dev_priv->buffers[num_buffers-1]->offset;
> +
> + DRM_DEBUG("i915 exec batchbuffer, start %x used %d cliprects %d\n",
> + batch->start, batch->used, batch->num_cliprects);
> +
> + ret = i915_dispatch_batchbuffer(dev, batch);
> + if (ret)
> + goto out_err0;
> +
> + sarea_priv->last_dispatch = READ_BREADCRUMB(dev_priv);
> +
> + /* fence */
> + ret = drm_fence_buffer_objects(dev, NULL, 0, NULL, &fence);
> + if (ret)
> + goto out_err0;
> +
> + if (!(fence_arg->flags & DRM_FENCE_FLAG_NO_USER)) {
> + ret = drm_fence_add_user_object(file_priv, fence, fence_arg->flags & DRM_FENCE_FLAG_SHAREABLE);
> + if (!ret) {
> + fence_arg->handle = fence->base.hash.key;
> + fence_arg->fence_class = fence->fence_class;
> + fence_arg->type = fence->type;
> + fence_arg->signaled = fence->signaled;
> + }
> + }
> + drm_fence_usage_deref_unlocked(&fence);
> +out_err0:
> +
> + /* handle errors */
> + mutex_lock(&dev->struct_mutex);
> + i915_dereference_buffers_locked(dev_priv->buffers, num_buffers);
> + mutex_unlock(&dev->struct_mutex);
> +
> + return ret;
> +}
> #define DRM_IOCTL_I915_INIT DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
> #define DRM_IOCTL_I915_FLUSH DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
> @@ -177,7 +178,7 @@ typedef struct _drm_i915_sarea {
> #define DRM_IOCTL_I915_SET_VBLANK_PIPE DRM_IOW( DRM_COMMAND_BASE + DRM_I915_SET_VBLANK_PIPE, drm_i915_vblank_pipe_t)
> #define DRM_IOCTL_I915_GET_VBLANK_PIPE DRM_IOR( DRM_COMMAND_BASE + DRM_I915_GET_VBLANK_PIPE, drm_i915_vblank_pipe_t)
> #define DRM_IOCTL_I915_VBLANK_SWAP DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_VBLANK_SWAP, drm_i915_vblank_swap_t)
> -
> +#define DRM_IOCTL_I915_EXECBUFFER DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_EXECBUFFER, struct drm_i915_execbuffer)
I know it's not in keeping with DRM tradition, but a short blurb about how
this ioctl works (i.e. data structures and how to submit them) would be
nice... :)
> @@ -325,4 +326,29 @@ typedef struct drm_i915_hws_addr {
> uint64_t addr;
> } drm_i915_hws_addr_t;
>
> +struct drm_i915_reloc {
> + uint32_t handle;
> + uint32_t offset;
> + uint32_t delta;
> +};
> +
> +#define I915_RELOC_TYPE_0 0
> +
> +struct drm_i915_op_arg {
> + uint64_t next;
> + uint32_t reloc_handle;
> + int handled;
> + union {
> + struct drm_bo_op_req req;
> + struct drm_bo_arg_rep rep;
> + } d;
> +
> +};
> +
> +struct drm_i915_execbuffer {
> + struct _drm_i915_batchbuffer batch;
> + uint64_t ops_list;
> + struct drm_fence_arg fence_arg;
> +};
Why the _ prefixed version here? In my tree there's no _drm_i915_batchbuffer,
just drm_i915_batchbuffer...
> diff --git a/shared-core/i915_drv.h b/shared-core/i915_drv.h
> index 3b26040..c86fd8a 100644
> --- a/shared-core/i915_drv.h
> +++ b/shared-core/i915_drv.h
> @@ -65,6 +65,10 @@
> #endif
> #define DRIVER_PATCHLEVEL 0
>
> +#ifdef I915_HAVE_BUFFER
> +#define I915_NUM_VALIDATE_BUFFERS 256
> +#endif
Why 256?
> +
> typedef struct _drm_i915_ring_buffer {
> int tail_mask;
> unsigned long Start;
> @@ -133,10 +137,12 @@ typedef struct drm_i915_private {
> #endif
> #ifdef I915_HAVE_BUFFER
> void *agp_iomap;
> + struct drm_buffer_object *buffers[I915_NUM_VALIDATE_BUFFERS];
> #endif
> DRM_SPINTYPE swaps_lock;
> drm_i915_vbl_swap_t vbl_swaps;
> unsigned int swaps_pending;
> +
> } drm_i915_private_t;
Nit: extra newline.
|