From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms Date: Sun, 4 Sep 2011 21:49:43 +0200 Message-ID: <20110904194942.GC2799@phenom.ffwll.local> References: <1314994047-4101-1-git-send-email-rob.clark@linaro.org> <20110903185355.GA2837@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ew0-f49.google.com (mail-ew0-f49.google.com [209.85.215.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 8C7D5A0DCD for ; Sun, 4 Sep 2011 12:49:25 -0700 (PDT) Received: by ewy3 with SMTP id 3so2342849ewy.36 for ; Sun, 04 Sep 2011 12:49:24 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Rob Clark Cc: inki.dae@samsung.com, linaro-dev@lists.linaro.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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 { > >> + =A0 =A0 struct list_head list; > >> + =A0 =A0 struct omap_gem_object *omap_obj; > >> + =A0 =A0 enum omap_gem_op op; > >> + =A0 =A0 uint32_t read_target, write_target; > >> + =A0 =A0 /* notify called w/ sync_lock held */ > >> + =A0 =A0 void (*notify)(void *arg); > >> + =A0 =A0 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 us= er > >> + * 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) > >> +{ > >> + =A0 =A0 struct omap_gem_object *omap_obj =3D waiter->omap_obj; > >> + =A0 =A0 if ((waiter->op & OMAP_GEM_READ) && > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (omap_obj->sync->read_comple= te < waiter->read_target)) > >> + =A0 =A0 =A0 =A0 =A0 =A0 return true; > >> + =A0 =A0 if ((waiter->op & OMAP_GEM_WRITE) && > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (omap_obj->sync->write_compl= ete < waiter->write_target)) > >> + =A0 =A0 =A0 =A0 =A0 =A0 return true; > >> + =A0 =A0 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