>
> 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...
You'd think that, and some parts maybe later could be generalised
internally in the kenrel, but the ioctl needs to remain driver specific
otherwise we won't be able to optimise use-cases for specific drivers..
Thomas's original code attempted to make parts of this generic in libdrm
but once I started experimenting with it I found it limited what I could
do.. the i915 has different requirements to the poulsbo which are the only
two example superioctls in the wild at the moment..
>> +#define RELOC_START_OFFSET 2
>> +#define RELOC_NUM_INTS 3
>
> These seem unused?
Good point, I had a plan.. then forgot it :-)
>> + 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.
Damn ia64 hackers...
> 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?
Yeah I thought about doing that, it's what poulsbo does pretty much, then
I also thought for future different relocs type you really just want
stride and pointer..
>> + 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...
We can probably remove thse later when we do init in-kernel.
>> +#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... :)
I'll have userspace example code in my mesa tree.. I can't see there
being many users beyond Mesa and the DDX..
>> + 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...
I was starting to dump some typedefs...
>> +#ifdef I915_HAVE_BUFFER
>> +#define I915_NUM_VALIDATE_BUFFERS 256
>> +#endif
>
> Why 256?
It was larger than 42.. I suppose I could use getparam to report this to
userspace so we could change it in theory later..
Dave.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
