On Sun, Sep 04, 2011 at 11:29:43AM -0500, Rob Clark wrote:
Above set of get/put functions seem to do very little. Drop them for the first round?
The intention is to do attach/detach_pages here.. and in case of get/put_paddr do remapping into TILER if the buffer isn't physically contiguous. (Although in the TILER case, we are seeing how/if we can fit this into the IOMMU framework.. so API's here are still in flux. Non-tiled buffers are a natural fit for IOMMU, I think... but tiled buffers, perhaps not.)
I wanted to at least get the right API's in place here, even though the implementation is still being sorted out.
If I've grepped that one correctly, there not (yet) used, so just confuse when reviewing. They're also easier to understand with actual users ;-)
I think that's even true (perhaps even more so) for userspace stuff - there's an enormous body of precedence for adding feature flags in drm land for such stuff.
[snip]
+/* Buffer Synchronization:
- */
+struct omap_gem_sync_waiter {
- struct list_head list;
- struct omap_gem_object *omap_obj;
- enum omap_gem_op op;
- uint32_t read_target, write_target;
- /* notify called w/ sync_lock held */
- void (*notify)(void *arg);
- void *arg;
+};
+/* list of omap_gem_sync_waiter.. the notify fxn gets called back when
- the read and/or write target count is achieved which can call a user
- callback (ex. to kick 3d and/or 2d), wakeup blocked task (prep for
- cpu access), etc.
- */
+static LIST_HEAD(waiters);
+static inline bool is_waiting(struct omap_gem_sync_waiter *waiter) +{
- struct omap_gem_object *omap_obj = waiter->omap_obj;
- if ((waiter->op & OMAP_GEM_READ) &&
- (omap_obj->sync->read_complete < waiter->read_target))
- return true;
- if ((waiter->op & OMAP_GEM_WRITE) &&
- (omap_obj->sync->write_complete < waiter->write_target))
- return true;
- return false;
+}
On a quick read this looks awfully like handrolled gpu sync objects. For which we already have a fully-featured implementation in ttm. And and something handrolled in i915 (request tracking). Can we do better?
[ Looks like it's part of the plugin layer, so problem postponed. Puhh ]
yeah, it is a bit handrolled sync-objects. I've looked a bit (although maybe not enough) at the TTM code, although not immediately sure how to do better. For better or for worse, some of the implementation (like the in-memory layout) is dictated by SGX. It's an area that I'm still working on and trying to figure out how to improve, but somehow has to coexist w/ SGX otherwise the page-flipping in the KMS part won't work.
My gripes aren't with the hw interfacing side but more with the wheel-reinventing for the signalling and boilerblate accounting code.
Which ttm has a complete framework for.
Up to now only i915 has been the odd man out, adding more is imo Not So Good (tm). Unfortunately there's no easy way out: Unifying ttm and i915 style gem is a hellalotta work, and growing gem into something like ttm is pretty pointless (which code like this will eventually lead to).
Inline with its only user above and scrap the forward decl?
omap_gem_new_handle? It is used both in omap_gem_dumb_create() and also ioctl_gem_new() for userspace creation of GEM buffers.. or where you talking about something else?
Oops, grepping gone wrong. Sorry for the noise.
Generally I think the harder issues are all with the plugin layer. And maybe I'll get my act toghether and clean the i915-gem vs ttm mess a bit up until you've got the first plugin merge-ready ;-)
Cheers, Daniel