Re: [PATCH 23/23] drm/ttm: remove bo->moving

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, 5 Apr 2022 at 11:25, Christian König <christian.koenig@amd.com> wrote:
>
> Am 29.03.22 um 18:02 schrieb Daniel Vetter:
> > On Mon, Mar 21, 2022 at 02:58:56PM +0100, Christian König wrote:
> > [SNIP]
> >>   static unsigned long ttm_bo_io_mem_pfn(struct ttm_buffer_object *bo,
> >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> >> index f999fdd927df..c6d02c98a19a 100644
> >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> >> @@ -1163,12 +1163,6 @@ int vmw_resources_clean(struct vmw_buffer_object *vbo, pgoff_t start,
> >>              *num_prefault = __KERNEL_DIV_ROUND_UP(last_cleaned - res_start,
> >>                                                    PAGE_SIZE);
> >>              vmw_bo_fence_single(bo, NULL);
> >> -            if (bo->moving)
> >> -                    dma_fence_put(bo->moving);
> >> -
> >> -            return dma_resv_get_singleton(bo->base.resv,
> >> -                                          DMA_RESV_USAGE_KERNEL,
> >> -                                          &bo->moving);
> > This seems to be entirely misplaced and I'm pretty sure doesn't even
> > compile interim.
>
> Mhm, removing that is correctly placed as far as I can see.
>
> What VMWGFX does here is to update bo->moving to please TTM, but since
> we now drop the bo->moving fence from TTM and always wait for all fences
> with DMA_RESV_USAGE_KERNEL before allowing CPU access that workaround
> isn't necessary any more.

Hm yeah that makes sense. Just out of paranoid would be good if you
can get an ack on the previous patch that downgrades from USAGE_WRITE
to USAGE_KERNEL here from vmwgfx folks, but I guess that should be
fine.

Just from reading the commit that introduce this is looks a little bit
like the intent is actually to make any USAGE_WRITE a mandatory fence
you can never cheat out of, but maybe I got this all wrong.

>
> >>      }
> >>
> >>      return 0;
> >> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> >> index c17b2df9178b..4c7134550262 100644
> >> --- a/include/drm/ttm/ttm_bo_api.h
> >> +++ b/include/drm/ttm/ttm_bo_api.h
> >> @@ -97,7 +97,6 @@ struct ttm_tt;
> >>    * @lru: List head for the lru list.
> >>    * @ddestroy: List head for the delayed destroy list.
> >>    * @swap: List head for swap LRU list.
> >> - * @moving: Fence set when BO is moving
> >>    * @offset: The current GPU offset, which can have different meanings
> >>    * depending on the memory type. For SYSTEM type memory, it should be 0.
> >>    * @cur_placement: Hint of current placement.
> >> @@ -150,7 +149,6 @@ struct ttm_buffer_object {
> >>       * Members protected by a bo reservation.
> >>       */
> >>
> >> -    struct dma_fence *moving;
> >>      unsigned priority;
> >>      unsigned pin_count;
> > Aside from the vmwgfx thing this looks good. With the vmwgfx patch split
> > issue (I think it's just that) fixed:
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Is it enough if I explain why we update VMWGFX in the commit message?

Yeah sounds fine to me, this patch isn't a functional change now that
you explained it that's clear. I'm still wondering whether there's
something fishy going on, but that's on earlier patches.
-Daniel

>
> Thanks,
> Christian.
>
> >
> >>
> >> --
> >> 2.25.1
> >>
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch