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
--
Daniel Vetter
Mail: daniel@xxxxxxxx
Mobile: +41 (0)79 365 57 48
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
