On 11.03.2013 09:18, Thierry Reding wrote:
This sound a bit over-engineered at this point in time. DRM is currently the only user. Is anybody working on any non-DRM drivers that would use this?
Well, this contains beginning of that:
http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=blob;f=drivers/media/vi...
I don't want to give these guys any excuse not to port it over to host1x code base. :-)
Even that aside, I don't think host1x_mem_handle is a good choice of name here. The objects are much more than handles. They are in fact buffer objects, which can optionally be attached to a handle. I also think that using a void * to store the handle specific data isn't such a good idea.
Naming if not an issue for me - we can easily agree on using _bo.
So how about the following proposal, which I think might satisfy both of us:
struct host1x_bo;
struct host1x_bo_ops { struct host1x_bo *(*get)(struct host1x_bo *bo); void (*put)(struct host1x_bo *bo); dma_addr_t (*pin)(struct host1x_bo *bo, struct sg_table **sgt); ... };
struct host1x_bo *host1x_bo_get(struct host1x_bo *bo); void host1x_bo_put(struct host1x_bo *bo); dma_addr_t host1x_bo_pin(struct host1x_bo *bo, struct sg_table **sgt); ...
struct host1x_bo { const struct host1x_bo_ops *ops; ... };
struct tegra_drm_bo { struct host1x_bo base; ... };
That way you can get rid of the host1x_memmgr_create_handle() helper and instead embed host1x_bo into driver-/framework-specific structures with the necessary initialization.
This would make sense. We'll get back when we have enough of implementation done to understand it all. One consequence is that we cannot use drm_gem_cma_create() anymore. We'll have to introduce a function that does the same as drm_gem_cma_create(), but it takes a pre-allocated drm_gem_cma_object pointer. That way we can allocate the struct, and use DRM CMA just to initialize the drm_gem_cma_object.
Other way would be just taking a copy of DRM CMA helper, but I'd like to defer that to the next step when we implement IOMMU aware allocator.
It also allows you to interact directly with the objects instead of having to go through the memmgr API. The memory manager doesn't really exist anymore so keeping the name in the API is only confusing. Your current proposal deals with memory handles directly already so it's really just making the naming more consistent.
The memmgr APIs are currently just a shortcut wrapper to the ops, so in that sense the memmgr does not really exist. I think it might still make sense to keep static inline wrappers for calling the ops within, but we could rename them to host1x_bo_somethingandother. Then it'd follow the pattern we are using for the hw ops in the latest set.
Terje