Re: [PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms

[Daniel Vetter <daniel@ffwll.ch>]

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@ffwll.ch
Mobile: +41 (0)79 365 57 48