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

[Rob Clark <rob.clark-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Sun, Sep 4, 2011 at 2:49 PM, Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org> wrote:
> 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 ;-)

The omap_fb.c stuff does a put_paddr() when it is done using a buffer
for scanout, fwiw.  But the put_paddr() stuff will make more sense
when I add support to remap discontiguous buffers for scanout.

> 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.

ahh, ok, I get your point.  When I started, I thought full blown TTM
would be overkill for a UMA setup.. but maybe I should revisit that.
Or if nothing else, see how to somehow refactor some of that out.. I'm
not really sure what the best thing to do would be at this point.
Although I suppose the gma500 driver would be in the same boat..

> 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).

yeah, that makes sense.. well, I'm open to suggestions here :-)

BR,
-R