[PATCH 1/4] drm: add prime helpers

[Daniel Vetter]

On Fri, Dec 07, 2012 at 09:58:38AM -0800, Aaron Plattner wrote:
> On 12/06/2012 01:46 PM, Daniel Vetter wrote:

[snip]

> >- Just an idea for all the essential noop cpu helpers: Maybe just call
> >   them dma_buf*noop and shovel them as convenience helpers into the
> >   dma-buf.c code in the core, for drivers that don't want/can't/won't
> >   bother to implement the cpu access support. Related: Exporting the
> >   functions in general so that drivers could pick&choose
> 
> Seems like a good idea as a follow-on change.  Do you think it would
> make more sense to have noop helpers, or allow them to be NULL in
> dma-buf.c?

Yeah, sounds good. Just thought that drm drivers aren't the only ones not
caring about cpu access support too much, so an officially blessed way
with documentation and unified errno would be good

[snip]

> >- 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;
}

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch