On 12/07/2012 10:48 AM, Daniel Vetter wrote:
On Fri, Dec 07, 2012 at 09:58:38AM -0800, Aaron Plattner wrote:
On 12/06/2012 01:46 PM, Daniel Vetter wrote:
- To make it a first-class helper and remove any and all midlayer stints
from this (I truly loath midlayers ...) maybe shovel this into a
drm_prime_helper.c file. Also, adding functions to the core vtable for
pure helpers is usually not too neat, since it makes it way too damn
easy to mix things up and transform the helper into a midlayer by
accident. E.g. the crtc helper functions all just use a generic void *
pointer for their vtables, other helpers either subclass an existing
struct or use their completely independent structs (which the driver can
both embed into it's own object struct and then do the right upclassing
in the callbacks). tbh haven't looked to closely at the series to asses
what would make sense, but we have way too much gunk in the drm vtable
already ...
I'm not sure I fully understand what you mean by this. Are you
suggesting creating a separate struct for these functions and having a
void* pointer to that in struct drm_driver?
Create a new struct like
struct drm_prime_helper_funcs {
int (*gem_prime_pin)(struct drm_gem_object *obj);
struct sg_table *(*gem_prime_get_pages)(struct drm_gem_object *obj);
struct drm_gem_object *(*gem_prime_import_sg)(struct drm_device *dev,
size_t size, struct sg_table *sg);
void *(*gem_prime_vmap)(struct drm_gem_object *obj);
void (*gem_prime_vunmap)(struct drm_gem_object *obj);
}
struct drm_prime_helper_obj {
/* vmap information */
int vmapping_count;
void *vmapping_ptr;
/* any other data the helpers need ... */
struct drm_prime_helper_funcs *funcs;
}
Ah, I see what you mean. This would also need either a pointer to the
drv directly so the helpers can lock drv->struct_mutex, or a helper
function to convert from a drm_prime_helper_obj* to a gem_buffer_object*
in a driver-specific way since the helper doesn't know the layout of the
driver's bo structure.
So it would be something like
struct drm_prime_helper_obj *helper_obj = dma_buf->priv;
struct drm_prime_helper_funcs *funcs = helper_obj->funcs;
struct drm_gem_object *obj = funcs->get_gem_obj(helper_obj);
mutex_lock(&obj->dev->struct_mutex);
funcs->whatever(obj);
mutex_unlock&obj->dev->struct_mutex);
and then for example, radeon_drm_prime_get_gem_obj would be
struct radeon_bo *bo = container_of(helper_obj, struct radeon_bo,
prime_helper);
return &bo->gem_base;
?
That seems a little over-engineered to me, but I can code it up.
--
Aaron
Drivers then fill out a func vtable and embedded the helper obj into their
own gpu bo struct, i.e.
struct radeon_bo {
struct ttm_bo ttm_bo;
struct gem_buffer_object gem_bo;
struct drm_prime_helper_obj prime_bo;
/* other stuff */
}
In the prime helper callbacks in the driver then upcast the prime_bo to
the radeon_bo instead of the gem_bo to the radeon bo.
Upsides: Guaranteed to not interfere with drivers not using it, with an
imo higher chance that the helper is cleanly separate from core stuff (no
guarantee ofc) and so easier to refactor in the future. And drm is full
with legacy stuff used by very few drivers, but highly intermingled with
drm core used by other drivers (my little holy war here is to slowly
refactor those things out and shovel them into drivers), hence why I
prefer to have an as clean separation of optional stuff as possible ...
Also, this way allows different drivers to use completely different helper
libraries for the same task, without those two helper libraries ever
interfering.
Downside: Adds a pointer to each bo struct for drivers using the helpers.
Given how big those tend to be, not too onerous. Given And maybe a bit of
confusion in driver callbacks, but since the linux container_of is
typechecked by the compiler, not too onerous either.
Dunno whether this aligns with what you want to achieve here. But on a
quick hunch this code feels rather unflexible and just refactors the
identical copypasta code back into one copy. Anyway, just figured I'll
drop my 2 cents here, feel free to ignore (maybe safe for the last one).
I think uncopypasta-ing the current code is beneficial on its own.
Do you think doing this now would make it harder to do the further
refactoring you suggest later?
If we later on go with something like the above, it'll require going over
all drivers again, doing similar mechanic changes. If no one yet managed
to intermingle the helpers with the core in the meantime, that is ;-)
Cheers, Daniel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
