On Sunday, October 7, 2007 4:26 pm Dave Airlie wrote:
> > 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..
Ah ok, we don't want to give up on optimizing for specific devices, so
if poulsbo and i915 already have different characteristics we'd better
keep the ioctls driver specific.
> >> + 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...
:) Don't forget that unaligned accesses on x86 cost a lot too, you just
don't take an interrupt for them...
> > 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..
Yeah, up to you. If you don't end up with a separate struct though, it
would be good to add a comment describing the layout.
> >> + 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.
Cool (my first reaction to anything configurable is "oh no, not another
config option", so I had to ask :).
> >> +#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..
Sounds good.
>
> >> + 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...
Yeah, I noticed the massive removal in the DRM tree, looks really nice.
I guess there's just some driver specific stuff left...
> >> +#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..
I don't know if 42 is better than 256... do we have any measurements
that would help us pick a good number or that would tell us we need to
make it a runtime option? Or maybe just part of the argument structure
that's passed in?
Jesse
-------------------------------------------------------------------------
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
