On 1/30/24 11:39, Daniel Vetter wrote: > On Thu, Jan 25, 2024 at 10:07:03AM +0100, Boris Brezillon wrote: >> On Fri, 5 Jan 2024 21:46:16 +0300 >> Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> wrote: >> >>> * >>> * This function Increases the use count and allocates the backing pages if >>> * use-count equals to zero. >>> + * >>> + * Note that this function doesn't pin pages in memory. If your driver >>> + * uses drm-shmem shrinker, then it's free to relocate pages to swap. >>> + * Getting pages only guarantees that pages are allocated, and not that >>> + * pages reside in memory. In order to pin pages use drm_gem_shmem_pin(). >> >> I still find this explanation confusing, if pages are allocated, they >> reside in memory. The only difference between drm_gem_shmem_get_pages() >> and drm_gem_shmem_pin_pages() is that the former lets the system >> reclaim the memory if the buffer is idle (no unsignalled fence attached >> to the dma_resv). >> >> We also need to describe the workflow for GEM validation (that's the >> TTM term for the swapin process happening when a GPU job is submitted). >> >> 1. Prepare the GPU job and initialize its fence >> 2. Lock the GEM resv >> 3. Add the GPU job fence to the resv object >> 4. If the GEM is evicted >> a. call drm_gem_shmem_swapin_locked() >> b. get the new sgt with drm_gem_shmem_get_pages_sgt_locked() >> c. repopulate the MMU table (driver internals) > > Might be good to explain where to call drm_sched_job_arm() here for > drivers using drm/sched, since that also needs to be at a very specific > point. Probably best to flesh out the details here by linking to the > relevant drm/sched and gpuvm functions as examples. > >> 5. Unlock the GEM dma_resv >> 6. Submit the GPU job >> >> With this sequence, the GEM pages are guaranteed to stay around until >> the GPU job is finished. > > Yeah I think the comment needs to explain how this ties together with > dma_resv locking and dma_resv fences, otherwise it just doesn't make much > sense. > > This holds even more so given that some of the earlier drivers derived > from i915-gem code (and i915-gem itself) use _pin() both for these more > permanent pinnings, and also to temporarily put the memory in place before > it all gets fenced and then unpinned&unlocked. > > So would be really good to have the sharpest possible nomeclatura here we > can get, and link between all the related concepts and functions in the > kerneldoc. > > Some overview flow like Boris sketched above in a DOC: section would also > be great. Thank you all for the feedback! I'll add all this doc in the next version -- Best regards, Dmitry
