Re: [RFC PATCH v3 07/11] drm, cgroup: Add TTM buffer allocation stats

["Welty, Brian" <brian.welty@intel.com>]

On 6/26/2019 11:01 PM, Daniel Vetter wrote:
> On Thu, Jun 27, 2019 at 12:06:13AM -0400, Kenny Ho wrote:
>> On Wed, Jun 26, 2019 at 12:12 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>>>
>>> I think with all the ttm refactoring going on I think we need to de-ttm
>>> the interface functions here a bit. With Gerd Hoffmans series you can just
>>> use a gem_bo pointer here, so what's left to do is have some extracted
>>> structure for tracking memory types. I think Brian Welty has some ideas
>>> for this, even in patch form. Would be good to keep him on cc at least for
>>> the next version. We'd need to explicitly hand in the ttm_mem_reg (or
>>> whatever the specific thing is going to be).
>>
>> I assume Gerd Hoffman's series you are referring to is this one?
>> https://www.spinics.net/lists/dri-devel/msg215056.html
> 
> There's a newer one, much more complete, but yes that's the work.
> 
>> I can certainly keep an eye out for Gerd's refactoring while
>> refactoring other parts of this RFC.
>>
>> I have added Brian and Gerd to the thread for awareness.
> 
> btw just realized that maybe building the interfaces on top of ttm_mem_reg
> is maybe not the best. That's what you're using right now, but in a way
> that's just the ttm internal detail of how the backing storage is
> allocated. I think the structure we need to abstract away is
> ttm_mem_type_manager, without any of the actual management details.
> 

Any de-ttm refactoring should probably not spam all the cgroups folks.
So I removed cgroups list.

As Daniel mentioned, some of us are looking at possible refactoring of TTM
for reuse in i915 driver.
Here is a brief summary of some ideas to be considered:

 1) refactor part of ttm_mem_type_manager into a new drm_mem_type_region.
    Really, should then move the array from ttm_bo_device.man[] into drm_device.

    Relevant to drm_cgroup, you could then perhaps access these stats through
    drm_device and don't need the mem_stats array in drmcgrp_device_resource.

  1a)  doing this right means replacing TTM_PL_XXX memory types with new DRM
     defines.  But could keep the TTM ones as redefinition of (new) DRM ones.
     Probably those private ones (TTM_PL_PRIV) make this difficult.

  All of the above could be eventually leveraged by the vram support being
  implemented now in i915 driver.
  
  2) refactor ttm_mem_reg + ttm_bus_placement into something generic for
     any GEM object,  maybe call it drm_gem_object_placement.
     ttm_mem_reg could remain as a wrapper for TTM drivers.
     This hasn't been broadly discussed with intel-gfx folks, so not sure
     this fits well into i915 or not.

     Relevant to drm_cgroup, maybe this function:
	drmcgrp_mem_track_move(struct ttm_buffer_object *old_bo, bool evict,
		struct ttm_mem_reg *new_mem)
     could potentially become:
        drmcgrp_mem_track_move(struct drm_gem_object *old_bo, bool evict,
		struct drm_gem_object_placement *new_place)

     Though from ttm_mem_reg, you look to only be using mem_type and size.
     I think Daniel is noting that ttm_mem_reg wasn't truly needed here, so
     you could just pass in the mem_type and size instead.

Would appreciate any feedback (positive or negative) on above....
Perhaps this should move to a new thread?   I could send out basic RFC
patches for (1) if helpful but as it touches all the TTM drivers, nice to
hear some feedback first.
Anyway, this doesn't necessarily need to block forward progress on drm_cgroup,
as refactoring into common base structures could happen incrementally.

Thanks,
-Brian
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel