[PATCH 6/7] drm/nouveau: embed gem object in nouveau_bo

[David Herrmann]

Hi

On Wed, Aug 14, 2013 at 4:31 PM, Maarten Lankhorst
<maarten.lankhorst at canonical.com> wrote:
> Op 14-08-13 15:07, David Herrmann schreef:
>> There is no reason to keep the gem object separately allocated. nouveau is
>> the last user of gem_obj->driver_private, so if we embed it, we can get
>> rid of 8bytes per gem-object.
>>
>> The implementation follows the radeon driver. bo->gem is only valid, iff
>> the bo was created via the gem helpers _and_ iff the user holds a valid
>> gem reference. That is, as the gem object holds a reference to the
>> nouveau_bo. If you use nouveau_ref() to gain a bo reference, you are not
>> guaranteed to also hold a gem reference. The gem object might get
>> destroyed after the last user drops the gem-ref via
>> drm_gem_object_unreference(). Use drm_gem_object_reference() to gain a
>> gem-reference.
>>
>> For debugging, we can use bo->gem.filp != NULL to test whether a gem-bo is
>> valid. However, this shouldn't be used for real functionality to avoid
>> gem-internal dependencies.
>>
>> Note that the implementation follows the previous style. However, we no
>> longer can check for bo->gem != NULL to test for a valid gem object. This
>> wasn't done before, so we should be safe now.
> I'm all for getting rid of it, but I think it's not thorough enough. :P

Does that mean you are in favor of this patch? Or do you want me to
rework it right now for v2? Because I'd like to see this cleanup
applied first so all TTM drivers work the same way. On top of that we
can then do whatever we want regarding TTM refcounts. I am willing to
spend some time on this.

> There's still a separate refcount in ttm for the bo, and I think it doesn't make sense to keep it like that.
> Instead of having that refcount in ttm, could it be put entirely in gem?
>
> ttm_buffer_object_transfer is the only time ttm creates a bo, and it's immediately destroyed, so instead
> of calling ttm_bo_unref it could call ttm_bo_release directly, in which case it doesn't matter that refcount
> is managed by gem.

The bo_vm code also takes bo references.

> Oh except for vmwgfx of course, I always forget that 'special' case.
>
> Maybe something like this in memory?
>
> struct {
>   struct ttm_buffer_object bo;
>   struct drm_gem_object gem; // Can be anything, as long as first member is a refcount, and no hole between ttm and this..
> };

Ugh.. This won't particularly win any beauty contest..

> This would allow ttm to do kref_put((kref_t*)(bo +1), driver->releasefn), where driver->releasefn has to call ttm_bo_release again.
> Unfortunately unless drm is fixed dma-buf is still going to be as buggy as before, not much I can do about that. :/
> But this is something for a future where dma-buf in drm doesn't suck..

This same issue came up with the vma-access-management patches. I
dislike adding hacks to TTM to allow accessing the surrounding GEM
object only to keep TTM gem-agnostic. We could fix all this with a
simple gem_object* pointer in the ttm-bo (or a flag indicating
container_of is valid).

Or we make gem TTM-aware by optionally embedding either a ttm_bo or a
kref object. Hm. Ideas welcome!

Regards
David