| Message ID | 20230208145319.397235-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/gem-vram: handle NULL bo->resource in move callback | expand |
Am 08.02.23 um 15:53 schrieb Matthew Auld: > The ttm BO now initially has NULL bo->resource, and leaves the driver > the handle that. However it looks like we forgot to handle that for > ttm_bo_move_memcpy() users, like with vram-gem, since it just silently > returns zero. This seems to then trigger warnings like: > > WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_gem_vram_helper.c:255 drm_gem_vram_offset (??:?) > > Fix this by calling move_null() if the new resource is TTM_PL_SYSTEM, > otherwise do the multi-hop sequence to ensure can safely call into > ttm_bo_move_memcpy(), since it might also need to clear the memory. > This should give the same behaviour as before. > > While we are here let's also treat calling ttm_bo_move_memcpy() with > NULL bo->resource as programmer error, where expectation is that upper > layers should now handle it. > > Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources during BO creation") > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Christian König <christian.koenig@amd.com> Oh, I wasn't aware that this broke at so many places. Especially radeon was tested earlier in the development of the patch set. Thanks for looking into that, the radeon patch has my rb and the rest of the series is Acked-by: Christian König <christian.koenig@amd.com>. Regards, Christian. > --- > drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++++++++++ > drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ++-- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > index d40b3edb52d0..0bea3df2a16d 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -916,6 +916,17 @@ static int bo_driver_move(struct ttm_buffer_object *bo, > { > struct drm_gem_vram_object *gbo; > > + if (!bo->resource) { > + if (new_mem->mem_type != TTM_PL_SYSTEM) { > + hop->mem_type = TTM_PL_SYSTEM; > + hop->flags = TTM_PL_FLAG_TEMPORARY; > + return -EMULTIHOP; > + } > + > + ttm_bo_move_null(bo, new_mem); > + return 0; > + } > + > gbo = drm_gem_vram_of_bo(bo); > > return drm_gem_vram_bo_driver_move(gbo, evict, ctx, new_mem); > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c > index d9d2b0903b22..fd9fd3d15101 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c > @@ -157,8 +157,8 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, > bool clear; > int ret = 0; > > - if (!src_mem) > - return 0; > + if (WARN_ON(!src_mem)) > + return -EINVAL; > > src_man = ttm_manager_type(bdev, src_mem->mem_type); > if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) ||
On 10/02/2023 11:03, Christian König wrote: > Am 08.02.23 um 15:53 schrieb Matthew Auld: >> The ttm BO now initially has NULL bo->resource, and leaves the driver >> the handle that. However it looks like we forgot to handle that for >> ttm_bo_move_memcpy() users, like with vram-gem, since it just silently >> returns zero. This seems to then trigger warnings like: >> >> WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_gem_vram_helper.c:255 >> drm_gem_vram_offset (??:?) >> >> Fix this by calling move_null() if the new resource is TTM_PL_SYSTEM, >> otherwise do the multi-hop sequence to ensure can safely call into >> ttm_bo_move_memcpy(), since it might also need to clear the memory. >> This should give the same behaviour as before. >> >> While we are here let's also treat calling ttm_bo_move_memcpy() with >> NULL bo->resource as programmer error, where expectation is that upper >> layers should now handle it. >> >> Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources during >> BO creation") >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Cc: Christian König <christian.koenig@amd.com> > > Oh, I wasn't aware that this broke at so many places. Especially radeon > was tested earlier in the development of the patch set. > > Thanks for looking into that, the radeon patch has my rb and the rest of > the series is Acked-by: Christian König <christian.koenig@amd.com>. Should we go ahead and land this? (minus patch 3 since that is already fixed by vmware folks). > > Regards, > Christian. > >> --- >> drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++++++++++ >> drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ++-- >> 2 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c >> b/drivers/gpu/drm/drm_gem_vram_helper.c >> index d40b3edb52d0..0bea3df2a16d 100644 >> --- a/drivers/gpu/drm/drm_gem_vram_helper.c >> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c >> @@ -916,6 +916,17 @@ static int bo_driver_move(struct >> ttm_buffer_object *bo, >> { >> struct drm_gem_vram_object *gbo; >> + if (!bo->resource) { >> + if (new_mem->mem_type != TTM_PL_SYSTEM) { >> + hop->mem_type = TTM_PL_SYSTEM; >> + hop->flags = TTM_PL_FLAG_TEMPORARY; >> + return -EMULTIHOP; >> + } >> + >> + ttm_bo_move_null(bo, new_mem); >> + return 0; >> + } >> + >> gbo = drm_gem_vram_of_bo(bo); >> return drm_gem_vram_bo_driver_move(gbo, evict, ctx, new_mem); >> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c >> b/drivers/gpu/drm/ttm/ttm_bo_util.c >> index d9d2b0903b22..fd9fd3d15101 100644 >> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c >> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c >> @@ -157,8 +157,8 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, >> bool clear; >> int ret = 0; >> - if (!src_mem) >> - return 0; >> + if (WARN_ON(!src_mem)) >> + return -EINVAL; >> src_man = ttm_manager_type(bdev, src_mem->mem_type); >> if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) || >
Am 21.02.23 um 17:13 schrieb Matthew Auld: > On 10/02/2023 11:03, Christian König wrote: >> Am 08.02.23 um 15:53 schrieb Matthew Auld: >>> The ttm BO now initially has NULL bo->resource, and leaves the driver >>> the handle that. However it looks like we forgot to handle that for >>> ttm_bo_move_memcpy() users, like with vram-gem, since it just silently >>> returns zero. This seems to then trigger warnings like: >>> >>> WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_gem_vram_helper.c:255 >>> drm_gem_vram_offset (??:?) >>> >>> Fix this by calling move_null() if the new resource is TTM_PL_SYSTEM, >>> otherwise do the multi-hop sequence to ensure can safely call into >>> ttm_bo_move_memcpy(), since it might also need to clear the memory. >>> This should give the same behaviour as before. >>> >>> While we are here let's also treat calling ttm_bo_move_memcpy() with >>> NULL bo->resource as programmer error, where expectation is that upper >>> layers should now handle it. >>> >>> Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources >>> during BO creation") >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>> Cc: Christian König <christian.koenig@amd.com> >> >> Oh, I wasn't aware that this broke at so many places. Especially >> radeon was tested earlier in the development of the patch set. >> >> Thanks for looking into that, the radeon patch has my rb and the rest >> of the series is Acked-by: Christian König <christian.koenig@amd.com>. > > Should we go ahead and land this? (minus patch 3 since that is already > fixed by vmware folks). Yeah, sure go ahead. Thanks, Christian. > >> >> Regards, >> Christian. >> >>> --- >>> drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++++++++++ >>> drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ++-- >>> 2 files changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c >>> b/drivers/gpu/drm/drm_gem_vram_helper.c >>> index d40b3edb52d0..0bea3df2a16d 100644 >>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c >>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c >>> @@ -916,6 +916,17 @@ static int bo_driver_move(struct >>> ttm_buffer_object *bo, >>> { >>> struct drm_gem_vram_object *gbo; >>> + if (!bo->resource) { >>> + if (new_mem->mem_type != TTM_PL_SYSTEM) { >>> + hop->mem_type = TTM_PL_SYSTEM; >>> + hop->flags = TTM_PL_FLAG_TEMPORARY; >>> + return -EMULTIHOP; >>> + } >>> + >>> + ttm_bo_move_null(bo, new_mem); >>> + return 0; >>> + } >>> + >>> gbo = drm_gem_vram_of_bo(bo); >>> return drm_gem_vram_bo_driver_move(gbo, evict, ctx, new_mem); >>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c >>> b/drivers/gpu/drm/ttm/ttm_bo_util.c >>> index d9d2b0903b22..fd9fd3d15101 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c >>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c >>> @@ -157,8 +157,8 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object >>> *bo, >>> bool clear; >>> int ret = 0; >>> - if (!src_mem) >>> - return 0; >>> + if (WARN_ON(!src_mem)) >>> + return -EINVAL; >>> src_man = ttm_manager_type(bdev, src_mem->mem_type); >>> if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) || >>
On 21/02/2023 16:17, Christian König wrote: > Am 21.02.23 um 17:13 schrieb Matthew Auld: >> On 10/02/2023 11:03, Christian König wrote: >>> Am 08.02.23 um 15:53 schrieb Matthew Auld: >>>> The ttm BO now initially has NULL bo->resource, and leaves the driver >>>> the handle that. However it looks like we forgot to handle that for >>>> ttm_bo_move_memcpy() users, like with vram-gem, since it just silently >>>> returns zero. This seems to then trigger warnings like: >>>> >>>> WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_gem_vram_helper.c:255 >>>> drm_gem_vram_offset (??:?) >>>> >>>> Fix this by calling move_null() if the new resource is TTM_PL_SYSTEM, >>>> otherwise do the multi-hop sequence to ensure can safely call into >>>> ttm_bo_move_memcpy(), since it might also need to clear the memory. >>>> This should give the same behaviour as before. >>>> >>>> While we are here let's also treat calling ttm_bo_move_memcpy() with >>>> NULL bo->resource as programmer error, where expectation is that upper >>>> layers should now handle it. >>>> >>>> Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources >>>> during BO creation") >>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>>> Cc: Christian König <christian.koenig@amd.com> >>> >>> Oh, I wasn't aware that this broke at so many places. Especially >>> radeon was tested earlier in the development of the patch set. >>> >>> Thanks for looking into that, the radeon patch has my rb and the rest >>> of the series is Acked-by: Christian König <christian.koenig@amd.com>. >> >> Should we go ahead and land this? (minus patch 3 since that is already >> fixed by vmware folks). > > Yeah, sure go ahead. I assume this has to go via some drm-misc type tree, for which I don't currently have commit rights. Can you help with merging this? > > Thanks, > Christian. > >> >>> >>> Regards, >>> Christian. >>> >>>> --- >>>> drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++++++++++ >>>> drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ++-- >>>> 2 files changed, 13 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c >>>> b/drivers/gpu/drm/drm_gem_vram_helper.c >>>> index d40b3edb52d0..0bea3df2a16d 100644 >>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c >>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c >>>> @@ -916,6 +916,17 @@ static int bo_driver_move(struct >>>> ttm_buffer_object *bo, >>>> { >>>> struct drm_gem_vram_object *gbo; >>>> + if (!bo->resource) { >>>> + if (new_mem->mem_type != TTM_PL_SYSTEM) { >>>> + hop->mem_type = TTM_PL_SYSTEM; >>>> + hop->flags = TTM_PL_FLAG_TEMPORARY; >>>> + return -EMULTIHOP; >>>> + } >>>> + >>>> + ttm_bo_move_null(bo, new_mem); >>>> + return 0; >>>> + } >>>> + >>>> gbo = drm_gem_vram_of_bo(bo); >>>> return drm_gem_vram_bo_driver_move(gbo, evict, ctx, new_mem); >>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c >>>> b/drivers/gpu/drm/ttm/ttm_bo_util.c >>>> index d9d2b0903b22..fd9fd3d15101 100644 >>>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c >>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c >>>> @@ -157,8 +157,8 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object >>>> *bo, >>>> bool clear; >>>> int ret = 0; >>>> - if (!src_mem) >>>> - return 0; >>>> + if (WARN_ON(!src_mem)) >>>> + return -EINVAL; >>>> src_man = ttm_manager_type(bdev, src_mem->mem_type); >>>> if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) || >>> >
Am 21.02.23 um 17:26 schrieb Matthew Auld: > On 21/02/2023 16:17, Christian König wrote: >> Am 21.02.23 um 17:13 schrieb Matthew Auld: >>> On 10/02/2023 11:03, Christian König wrote: >>>> Am 08.02.23 um 15:53 schrieb Matthew Auld: >>>>> The ttm BO now initially has NULL bo->resource, and leaves the driver >>>>> the handle that. However it looks like we forgot to handle that for >>>>> ttm_bo_move_memcpy() users, like with vram-gem, since it just >>>>> silently >>>>> returns zero. This seems to then trigger warnings like: >>>>> >>>>> WARNING: CPU: 0 PID: 1 at >>>>> drivers/gpu/drm/drm_gem_vram_helper.c:255 drm_gem_vram_offset (??:?) >>>>> >>>>> Fix this by calling move_null() if the new resource is TTM_PL_SYSTEM, >>>>> otherwise do the multi-hop sequence to ensure can safely call into >>>>> ttm_bo_move_memcpy(), since it might also need to clear the memory. >>>>> This should give the same behaviour as before. >>>>> >>>>> While we are here let's also treat calling ttm_bo_move_memcpy() with >>>>> NULL bo->resource as programmer error, where expectation is that >>>>> upper >>>>> layers should now handle it. >>>>> >>>>> Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources >>>>> during BO creation") >>>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>>>> Cc: Christian König <christian.koenig@amd.com> >>>> >>>> Oh, I wasn't aware that this broke at so many places. Especially >>>> radeon was tested earlier in the development of the patch set. >>>> >>>> Thanks for looking into that, the radeon patch has my rb and the >>>> rest of the series is Acked-by: Christian König >>>> <christian.koenig@amd.com>. >>> >>> Should we go ahead and land this? (minus patch 3 since that is >>> already fixed by vmware folks). >> >> Yeah, sure go ahead. > > I assume this has to go via some drm-misc type tree, for which I don't > currently have commit rights. Can you help with merging this? Sure, going to push the series tomorrow. Thanks, Christian. > >> >> Thanks, >> Christian. >> >>> >>>> >>>> Regards, >>>> Christian. >>>> >>>>> --- >>>>> drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++++++++++ >>>>> drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ++-- >>>>> 2 files changed, 13 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c >>>>> b/drivers/gpu/drm/drm_gem_vram_helper.c >>>>> index d40b3edb52d0..0bea3df2a16d 100644 >>>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c >>>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c >>>>> @@ -916,6 +916,17 @@ static int bo_driver_move(struct >>>>> ttm_buffer_object *bo, >>>>> { >>>>> struct drm_gem_vram_object *gbo; >>>>> + if (!bo->resource) { >>>>> + if (new_mem->mem_type != TTM_PL_SYSTEM) { >>>>> + hop->mem_type = TTM_PL_SYSTEM; >>>>> + hop->flags = TTM_PL_FLAG_TEMPORARY; >>>>> + return -EMULTIHOP; >>>>> + } >>>>> + >>>>> + ttm_bo_move_null(bo, new_mem); >>>>> + return 0; >>>>> + } >>>>> + >>>>> gbo = drm_gem_vram_of_bo(bo); >>>>> return drm_gem_vram_bo_driver_move(gbo, evict, ctx, new_mem); >>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c >>>>> b/drivers/gpu/drm/ttm/ttm_bo_util.c >>>>> index d9d2b0903b22..fd9fd3d15101 100644 >>>>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c >>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c >>>>> @@ -157,8 +157,8 @@ int ttm_bo_move_memcpy(struct >>>>> ttm_buffer_object *bo, >>>>> bool clear; >>>>> int ret = 0; >>>>> - if (!src_mem) >>>>> - return 0; >>>>> + if (WARN_ON(!src_mem)) >>>>> + return -EINVAL; >>>>> src_man = ttm_manager_type(bdev, src_mem->mem_type); >>>>> if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) || >>>> >>
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index d40b3edb52d0..0bea3df2a16d 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -916,6 +916,17 @@ static int bo_driver_move(struct ttm_buffer_object *bo, { struct drm_gem_vram_object *gbo; + if (!bo->resource) { + if (new_mem->mem_type != TTM_PL_SYSTEM) { + hop->mem_type = TTM_PL_SYSTEM; + hop->flags = TTM_PL_FLAG_TEMPORARY; + return -EMULTIHOP; + } + + ttm_bo_move_null(bo, new_mem); + return 0; + } + gbo = drm_gem_vram_of_bo(bo); return drm_gem_vram_bo_driver_move(gbo, evict, ctx, new_mem); diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index d9d2b0903b22..fd9fd3d15101 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -157,8 +157,8 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, bool clear; int ret = 0; - if (!src_mem) - return 0; + if (WARN_ON(!src_mem)) + return -EINVAL; src_man = ttm_manager_type(bdev, src_mem->mem_type); if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) ||
The ttm BO now initially has NULL bo->resource, and leaves the driver the handle that. However it looks like we forgot to handle that for ttm_bo_move_memcpy() users, like with vram-gem, since it just silently returns zero. This seems to then trigger warnings like: WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_gem_vram_helper.c:255 drm_gem_vram_offset (??:?) Fix this by calling move_null() if the new resource is TTM_PL_SYSTEM, otherwise do the multi-hop sequence to ensure can safely call into ttm_bo_move_memcpy(), since it might also need to clear the memory. This should give the same behaviour as before. While we are here let's also treat calling ttm_bo_move_memcpy() with NULL bo->resource as programmer error, where expectation is that upper layers should now handle it. Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources during BO creation") Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++++++++++ drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-)