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

[Daniel Vetter]

On Fri, Jun 28, 2019 at 3:16 AM Welty, Brian <brian.welty at intel.com> wrote:
> 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 at 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.

Yeah I think the relevant part of your refactoring is creating a more
abstract memory type/resource thing (not the individual allocations
from it which ttm calls regions and I get confused about that every
time). I think that abstraction should also have a field for the total
(which I think cgroups also needs, both as read-only information and
starting value). ttm would that put somewhere into the
ttm_mem_type_manager, i915 would put it somewhere else, cma based
drivers could perhaps expose the cma heap like that (if it's exclusive
to the gpu at least).

> 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.

Yeah I think new dri-devel thread with a totally unpolished rfc as
draft plus this as the intro would be good. That way we can ground
this a bit better in actual code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch