[PATCH] RFC: omapdrm DRM/KMS driver for TI OMAP platforms
Rob Clark
rob.clark at linaro.org
Sun Sep 4 09:29:43 PDT 2011
On Sat, Sep 3, 2011 at 1:53 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> Hi Rob,
>
> I've taken a look at your driver, some comments inline. Can't really
> comment much on the kms side because of lack of expertise in that area.
>
> I think most can be trivially addressed by dropping the plugin layer ;-)
>
> Cheers, Daniel
>
> On Fri, Sep 02, 2011 at 03:07:27PM -0500, Rob Clark wrote:
>
>> [snip]
>
>> +int omap_connector_sync(struct drm_connector *connector)
>> +{
>> + struct omap_connector *omap_connector = to_omap_connector(connector);
>> + struct omap_dss_device *dssdev = omap_connector->dssdev;
>> + struct omap_dss_driver *dssdrv = dssdev->driver;
>> +
>> + DBG("%s", omap_connector->dssdev->name);
>> +
>> + if (dssdrv->sync) {
>> + return dssdrv->sync(dssdev);
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(omap_connector_sync);
>
> What is this for? There seem to be a few other EXPORT_SYMBOLS (beside
> those to register plugins) that look a bit strange - especially since
> there already is the dss layer to arbitrage between all the drivers (drm,
> v4l, fbdev). Or is that all stuff needed by your plugins? If so, better
> drop them until the first user shows up (and even then it's likely easer
> if the glue code comes as a separate patch).
this particular one is a remnant from an earlier version, where page
flipping and vsync sync'ing was handled in the PVR code.. once I
started adding sync-obj I've moved this into omap_crtc using the
normal page-flip ioctl.. it looks like I missed a bit of cleanup of
some of these fxns which are no longer needed since I moved the page
flipping into the KMS code.
At this point, I think it is just the exported symbols in the GEM bits
that are needed by the PVR plugin (and at least some would be needed
by other upcoming plugins for video encode/decode and then 2d).
>> [snip]
>
>> +/** ensure backing pages are allocated */
>> +/* NOTE: a bit similar to psb_gtt_attach_pages().. perhaps could be common?
>> + * and maybe a bit similar to i915_gem_object_get_pages_gtt()
>> + */
>
> Actually see also i915_gem_object_get_pages_gtt for yet another copy of
> this. This is a bit a mess and I'm not sure what to do, safe to slowly
> evolve gem into something resembling ttm ...
Yeah, I'd like to refactor out the attach/detach_pages stuff.. it is
on my todo list (I guess I should at least mention that in the TODO
file..)
>> [snip]
>
>> + * already contiguous, remap it to pin in physically contiguous memory.. (ie.
>> + * map in TILER)
>> + */
>> +int omap_gem_get_paddr(struct drm_gem_object *obj,
>> + unsigned long *paddr, bool remap)
>> +{
>> + struct omap_gem_object *omap_obj = to_omap_bo(obj);
>> +
>> + if (!(omap_obj->flags & OMAP_BO_DMA)) {
>> + /* TODO: remap to TILER */
>> + return -ENOMEM;
>> + }
>> +
>> + *paddr = omap_obj->paddr;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(omap_gem_get_paddr);
>> +
>> +/* Release physical address, when DMA is no longer being performed.. this
>> + * could potentially unpin and unmap buffers from TILER
>> + */
>> +int omap_gem_put_paddr(struct drm_gem_object *obj)
>> +{
>> + /* do something here when remap to TILER is used.. */
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(omap_gem_put_paddr);
>> +
>> +/* acquire pages when needed (for example, for DMA where physically
>> + * contiguous buffer is not required
>> + */
>> +int omap_gem_get_pages(struct drm_gem_object *obj, struct page ***pages)
>> +{
>> + struct omap_gem_object *omap_obj = to_omap_bo(obj);
>> + /* TODO: we could attach/detach pages on demand */
>> + int ret; // XXX below is common in _fault()..
>> + if (obj->filp && !omap_obj->pages) {
>> + ret = omap_gem_attach_pages(obj);
>> + if (ret) {
>> + dev_err(obj->dev->dev, "could not attach pages\n");
>> + return ret;
>> + }
>> + }
>> + /* TODO: even phys-contig.. we should have a list of pages */
>> + *pages = omap_obj->pages;
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(omap_gem_get_pages);
>> +
>> +/* release pages when DMA no longer being performed */
>> +int omap_gem_put_pages(struct drm_gem_object *obj)
>> +{
>> + /* do something here if we dynamically attach/detach pages.. at
>> + * least they would no longer need to be pinned if everyone has
>> + * released the pages..
>> + */
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(omap_gem_put_pages);
>
> 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.
>> [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.
>> [snip]
>
>> +void omap_gem_set_sync_object(struct drm_gem_object *obj, void *syncobj)
>> +{
>> + struct omap_gem_object *omap_obj = to_omap_bo(obj);
>> +
>> + spin_lock(&sync_lock);
>> +
>> + if ((omap_obj->flags & OMAP_BO_EXT_SYNC) && !syncobj) {
>> + /* clearing a previously set syncobj */
>> + syncobj = kzalloc(sizeof(*omap_obj->sync), GFP_KERNEL|GFP_ATOMIC);
>> + memcpy(syncobj, omap_obj->sync, sizeof(*omap_obj->sync));
>> + omap_obj->flags &= ~OMAP_BO_EXT_SYNC;
>> + omap_obj->sync = syncobj;
>> + } else if (syncobj && !(omap_obj->flags & OMAP_BO_EXT_SYNC)) {
>> + /* replacing an existing syncobj */
>> + if (omap_obj->sync) {
>> + memcpy(syncobj, omap_obj->sync, sizeof(*omap_obj->sync));
>> + kfree(omap_obj->sync);
>> + }
>> + omap_obj->flags |= OMAP_BO_EXT_SYNC;
>> + omap_obj->sync = syncobj;
>> + }
>> + spin_unlock(&sync_lock);
>> +}
>> +EXPORT_SYMBOL(omap_gem_set_sync_object);
>
> Abstraction layers to adapt some blobs interface (be it the Windows kernel
> driver interface or a blob driver core) is a bit frowned upon (actually I
> think some people would nuke them first, ask questions later ;-). Maybe
> hide that in your own vendor patch?
just fwiw, this is a bit of a symptom of how sync-objects get mapped
to SGX's MMU.. we can't really afford to burn one page per
sync-object, so when GEM objects get mapped to the GPU it might be
required to relocate the sync-object. It is more a result of how SGX
firmware works. (At least nothing to do w/ drivers from other
operating systems.) Of course, life would be much nicer for me if the
rest of the IMG userspace stuff was open, but I'm just trying to do
the best with what I've got. But even if it was open, I think it
would still have to work the same way. (The kernel part of pvr is
GPL, fwiw.. not that it helps much here.)
>> +int omap_gem_init_object(struct drm_gem_object *obj)
>> +{
>> + return -EINVAL; /* unused */
>> +}
>> +
>> +/* don't call directly.. called from GEM core when it is time to actually
>> + * free the object..
>> + */
>> +void omap_gem_free_object(struct drm_gem_object *obj)
>> +{
>> + struct omap_gem_object *omap_obj = to_omap_bo(obj);
>> +
>> + if (obj->map_list.map) {
>> + drm_gem_free_mmap_offset(obj);
>> + }
>> +
>> + /* don't free externally allocated backing memory */
>> + if (!(omap_obj->flags & OMAP_BO_EXT_MEM)) {
>
>
> Nobody sets OMAP_BO_EXT_MEM flag. What's this for? Better implement this
> when it actually gets used ...
>
This is used by "the plugin".. which needs a way to share the same
mmap(), so other things that get mapped to userspace end up getting
wrapped as a GEM object..
>> + if (!obj->filp) {
>> + /* TODO: is this the best way to check? */
>> + omap_vram_free(omap_obj->paddr, obj->size);
>> + }
>> +
>> + if (omap_obj->pages) {
>> + omap_gem_detach_pages(obj);
>> + }
>> + }
>> +
>> + /* don't free externally allocated syncobj */
>> + if (!(omap_obj->flags & OMAP_BO_EXT_SYNC)) {
>> + kfree(omap_obj->sync);
>> + }
>> +
>> + drm_gem_object_release(obj);
>> +
>> + kfree(obj);
>> +}
>> +
>> +/* convenience method to construct a GEM buffer object, and userspace handle */
>
> 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?
>> +int omap_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>> + size_t size, uint32_t flags, uint32_t *handle, uint64_t *offset)
>> +{
>> + struct drm_gem_object *obj;
>> + int ret;
>> +
>> + obj = omap_gem_new(dev, size, flags);
>> + if (!obj)
>> + return -ENOMEM;
>> +
>> + ret = drm_gem_handle_create(file, obj, handle);
>> + if (ret) {
>> + drm_gem_object_release(obj);
>> + kfree(obj); /* TODO isn't there a dtor to call? just copying i915 */
>> + return ret;
>> + }
>> +
>> + if (offset) {
>> + *offset = omap_gem_mmap_offset(obj);
>> + }
>> +
>> + /* drop reference from allocate - handle holds it now */
>> + drm_gem_object_unreference_unlocked(obj);
>> +
>> + return 0;
>> +}
>> +/* common constructor body */
>> +static struct drm_gem_object * omap_gem_new_impl(struct drm_device *dev,
>> + size_t size, uint32_t flags, unsigned long paddr, struct page **pages,
>> + struct omap_gem_vm_ops *ops)
>> +{
>> + struct omap_gem_object *omap_obj;
>> + struct drm_gem_object *obj;
>> + int ret;
>> +
>> + size = PAGE_ALIGN(size);
>> +
>> + omap_obj = kzalloc(sizeof(*omap_obj), GFP_KERNEL);
>> + if (!omap_obj) {
>> + dev_err(dev->dev, "could not allocate GEM object\n");
>> + goto fail;
>> + }
>> +
>> + obj = &omap_obj->base;
>> +
>> + if (paddr) {
>> + flags |= OMAP_BO_DMA;
>> + }
>> +
>> + omap_obj->paddr = paddr;
>> + omap_obj->pages = pages;
>> + omap_obj->flags = flags;
>> + omap_obj->ops = ops;
>> +
>> + if (flags & (OMAP_BO_DMA|OMAP_BO_EXT_MEM)) {
>> + ret = drm_gem_private_object_init(dev, obj, size);
>> + } else {
>> + ret = drm_gem_object_init(dev, obj, size);
>> + }
>> +
>> + if (ret) {
>> + /* Yikes! need to clean up! XXX */
>> + goto fail;
>> + }
>> +
>> + /* Make it mmapable */
>> + ret = drm_gem_create_mmap_offset(obj);
>> + if (ret) {
>> + dev_err(dev->dev, "could not allocate mmap offset");
>
> The drm mmap offset manager isn't the fatest beast in the world. Maybe
> create mmappings on demand only when userspace actually needs them? I.e.
> in the ioctl_gem_info function.
ok.. initially I didn't have DRM_IOCTL_OMAP_GEM_INFO so I was
returning the offset in DRM_IOCTL_OMAP_GEM_NEW, but that is a good
point that this is no longer required. I'll remove it from
DRM_IOCTL_OMAP_GEM_NEW and the constructor.
BR,
-R
>> + goto fail;
>> + }
>> +
>> + return obj;
>> +
>> +fail:
>> + if (omap_obj) {
>> + kfree(omap_obj);
>> + }
>> + return NULL;
>> +}
>> +
>
>> [snip]
> --
> Daniel Vetter
> Mail: daniel at ffwll.ch
> Mobile: +41 (0)79 365 57 48
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
More information about the dri-devel
mailing list