| Message ID | 20210915185954.3114858-6-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,01/12] drm/ttm: stop setting page->index for the ttm_tt | expand |
Am 15.09.21 um 20:59 schrieb Matthew Auld: > In commit: > > commit 667a50db0477d47fdff01c666f5ee1ce26b5264c > Author: Thomas Hellstrom <thellstrom@vmware.com> > Date: Fri Jan 3 11:17:18 2014 +0100 > > drm/ttm: Refuse to fault (prime-) imported pages > > we introduced the restriction that imported pages should not be directly > mappable through TTM(this also extends to userptr). In the next patch we > want to introduce a shmem_tt backend, which should follow all the > existing rules with TTM_PAGE_FLAG_EXTERNAL, since it will need to handle > swapping itself, but with the above mapping restriction lifted. > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Cc: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/ttm/ttm_bo_vm.c | 6 ++++-- > include/drm/ttm/ttm_tt.h | 7 +++++++ > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c > index 708390588c7c..fd6e18f12f50 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c > +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c > @@ -163,8 +163,10 @@ vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo, > * (if at all) by redirecting mmap to the exporter. > */ > if (bo->ttm && (bo->ttm->page_flags & TTM_PAGE_FLAG_EXTERNAL)) { > - dma_resv_unlock(bo->base.resv); > - return VM_FAULT_SIGBUS; > + if (!(bo->ttm->page_flags & TTM_PAGE_FLAG_EXTERNAL_MAPPABLE)) { > + dma_resv_unlock(bo->base.resv); > + return VM_FAULT_SIGBUS; > + } > } > > return 0; > diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h > index 7f54a83c95ef..800c9edb3e10 100644 > --- a/include/drm/ttm/ttm_tt.h > +++ b/include/drm/ttm/ttm_tt.h > @@ -66,11 +66,18 @@ struct ttm_tt { > * Note that enum ttm_bo_type.ttm_bo_type_sg objects will always enable > * this flag. > * > + * TTM_PAGE_FLAG_EXTERNAL_MAPPABLE: Same behaviour as > + * TTM_PAGE_FLAG_EXTERNAL, but with the reduced restriction that it is > + * still valid to use TTM to map the pages directly. This is useful when > + * implementing a ttm_tt backend which still allocates driver owned > + * pages underneath(say with shmem). > + * > * TTM_PAGE_FLAG_PRIV_POPULATED: TTM internal only. DO NOT USE. > */ > #define TTM_PAGE_FLAG_SWAPPED (1 << 0) > #define TTM_PAGE_FLAG_ZERO_ALLOC (1 << 1) > #define TTM_PAGE_FLAG_EXTERNAL (1 << 2) > +#define TTM_PAGE_FLAG_EXTERNAL_MAPPABLE (1 << 3 | TTM_PAGE_FLAG_EXTERNAL) That's really bad practice because an "if (!(flags & TTM_PAGE_FLAG_EXTERNAL_MAPPABLE))" has a different semantics as an "if (flags & TTM_PAGE_FLAG_EXTERNAL_MAPPABLE)". Rather add a TTM_PAGE_FLAG_UNMAPPABLE and make sure that it is set as appropriated. Regards, Christian. > > #define TTM_PAGE_FLAG_PRIV_POPULATED (1 << 31) > uint32_t page_flags;
On Thu, 2021-09-16 at 08:55 +0200, Christian König wrote: > > > Am 15.09.21 um 20:59 schrieb Matthew Auld: > > In commit: > > > > commit 667a50db0477d47fdff01c666f5ee1ce26b5264c > > Author: Thomas Hellstrom <thellstrom@vmware.com> > > Date: Fri Jan 3 11:17:18 2014 +0100 > > > > drm/ttm: Refuse to fault (prime-) imported pages > > > > we introduced the restriction that imported pages should not be > > directly > > mappable through TTM(this also extends to userptr). In the next > > patch we > > want to introduce a shmem_tt backend, which should follow all the > > existing rules with TTM_PAGE_FLAG_EXTERNAL, since it will need to > > handle > > swapping itself, but with the above mapping restriction lifted. > > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > Cc: Christian König <christian.koenig@amd.com> > > --- > > drivers/gpu/drm/ttm/ttm_bo_vm.c | 6 ++++-- > > include/drm/ttm/ttm_tt.h | 7 +++++++ > > 2 files changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c > > b/drivers/gpu/drm/ttm/ttm_bo_vm.c > > index 708390588c7c..fd6e18f12f50 100644 > > --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c > > +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c > > @@ -163,8 +163,10 @@ vm_fault_t ttm_bo_vm_reserve(struct > > ttm_buffer_object *bo, > > * (if at all) by redirecting mmap to the exporter. > > */ > > if (bo->ttm && (bo->ttm->page_flags & > > TTM_PAGE_FLAG_EXTERNAL)) { > > - dma_resv_unlock(bo->base.resv); > > - return VM_FAULT_SIGBUS; > > + if (!(bo->ttm->page_flags & > > TTM_PAGE_FLAG_EXTERNAL_MAPPABLE)) { I saw there was previously a thought about testing for ttm_bo_type_sg here. Was that a dead end? Thanks, Thomas
On 16/09/2021 10:03, Thomas Hellström wrote: > On Thu, 2021-09-16 at 08:55 +0200, Christian König wrote: >> >> >> Am 15.09.21 um 20:59 schrieb Matthew Auld: >>> In commit: >>> >>> commit 667a50db0477d47fdff01c666f5ee1ce26b5264c >>> Author: Thomas Hellstrom <thellstrom@vmware.com> >>> Date: Fri Jan 3 11:17:18 2014 +0100 >>> >>> drm/ttm: Refuse to fault (prime-) imported pages >>> >>> we introduced the restriction that imported pages should not be >>> directly >>> mappable through TTM(this also extends to userptr). In the next >>> patch we >>> want to introduce a shmem_tt backend, which should follow all the >>> existing rules with TTM_PAGE_FLAG_EXTERNAL, since it will need to >>> handle >>> swapping itself, but with the above mapping restriction lifted. >>> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> Cc: Christian König <christian.koenig@amd.com> >>> --- >>> drivers/gpu/drm/ttm/ttm_bo_vm.c | 6 ++++-- >>> include/drm/ttm/ttm_tt.h | 7 +++++++ >>> 2 files changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c >>> b/drivers/gpu/drm/ttm/ttm_bo_vm.c >>> index 708390588c7c..fd6e18f12f50 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c >>> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c >>> @@ -163,8 +163,10 @@ vm_fault_t ttm_bo_vm_reserve(struct >>> ttm_buffer_object *bo, >>> * (if at all) by redirecting mmap to the exporter. >>> */ >>> if (bo->ttm && (bo->ttm->page_flags & >>> TTM_PAGE_FLAG_EXTERNAL)) { >>> - dma_resv_unlock(bo->base.resv); >>> - return VM_FAULT_SIGBUS; >>> + if (!(bo->ttm->page_flags & >>> TTM_PAGE_FLAG_EXTERNAL_MAPPABLE)) { > > I saw there was previously a thought about testing for ttm_bo_type_sg > here. Was that a dead end? I guess the issue was userptr, which is also EXTERNAL and needs to be rejected here, but underneath it's not type_sg. Just having an explicit MAPPABLE flag or similar seemed the more flexible approach? Happy to try out other ideas. > > Thanks, > Thomas > >
On 9/16/21 11:58 AM, Matthew Auld wrote: > On 16/09/2021 10:03, Thomas Hellström wrote: >> On Thu, 2021-09-16 at 08:55 +0200, Christian König wrote: >>> >>> >>> Am 15.09.21 um 20:59 schrieb Matthew Auld: >>>> In commit: >>>> >>>> commit 667a50db0477d47fdff01c666f5ee1ce26b5264c >>>> Author: Thomas Hellstrom <thellstrom@vmware.com> >>>> Date: Fri Jan 3 11:17:18 2014 +0100 >>>> >>>> drm/ttm: Refuse to fault (prime-) imported pages >>>> >>>> we introduced the restriction that imported pages should not be >>>> directly >>>> mappable through TTM(this also extends to userptr). In the next >>>> patch we >>>> want to introduce a shmem_tt backend, which should follow all the >>>> existing rules with TTM_PAGE_FLAG_EXTERNAL, since it will need to >>>> handle >>>> swapping itself, but with the above mapping restriction lifted. >>>> >>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>>> Cc: Christian König <christian.koenig@amd.com> >>>> --- >>>> drivers/gpu/drm/ttm/ttm_bo_vm.c | 6 ++++-- >>>> include/drm/ttm/ttm_tt.h | 7 +++++++ >>>> 2 files changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c >>>> b/drivers/gpu/drm/ttm/ttm_bo_vm.c >>>> index 708390588c7c..fd6e18f12f50 100644 >>>> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c >>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c >>>> @@ -163,8 +163,10 @@ vm_fault_t ttm_bo_vm_reserve(struct >>>> ttm_buffer_object *bo, >>>> * (if at all) by redirecting mmap to the exporter. >>>> */ >>>> if (bo->ttm && (bo->ttm->page_flags & >>>> TTM_PAGE_FLAG_EXTERNAL)) { >>>> - dma_resv_unlock(bo->base.resv); >>>> - return VM_FAULT_SIGBUS; >>>> + if (!(bo->ttm->page_flags & >>>> TTM_PAGE_FLAG_EXTERNAL_MAPPABLE)) { >> >> I saw there was previously a thought about testing for ttm_bo_type_sg >> here. Was that a dead end? > > I guess the issue was userptr, which is also EXTERNAL and needs to be > rejected here, but underneath it's not type_sg. OK, I thought userptr was using type_sg. I wonder what it's for if that's not the case... > > Just having an explicit MAPPABLE flag or similar seemed the more > flexible approach? Happy to try out other ideas. > I'm fine with a flag as well. /Thomas
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index 708390588c7c..fd6e18f12f50 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -163,8 +163,10 @@ vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo, * (if at all) by redirecting mmap to the exporter. */ if (bo->ttm && (bo->ttm->page_flags & TTM_PAGE_FLAG_EXTERNAL)) { - dma_resv_unlock(bo->base.resv); - return VM_FAULT_SIGBUS; + if (!(bo->ttm->page_flags & TTM_PAGE_FLAG_EXTERNAL_MAPPABLE)) { + dma_resv_unlock(bo->base.resv); + return VM_FAULT_SIGBUS; + } } return 0; diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h index 7f54a83c95ef..800c9edb3e10 100644 --- a/include/drm/ttm/ttm_tt.h +++ b/include/drm/ttm/ttm_tt.h @@ -66,11 +66,18 @@ struct ttm_tt { * Note that enum ttm_bo_type.ttm_bo_type_sg objects will always enable * this flag. * + * TTM_PAGE_FLAG_EXTERNAL_MAPPABLE: Same behaviour as + * TTM_PAGE_FLAG_EXTERNAL, but with the reduced restriction that it is + * still valid to use TTM to map the pages directly. This is useful when + * implementing a ttm_tt backend which still allocates driver owned + * pages underneath(say with shmem). + * * TTM_PAGE_FLAG_PRIV_POPULATED: TTM internal only. DO NOT USE. */ #define TTM_PAGE_FLAG_SWAPPED (1 << 0) #define TTM_PAGE_FLAG_ZERO_ALLOC (1 << 1) #define TTM_PAGE_FLAG_EXTERNAL (1 << 2) +#define TTM_PAGE_FLAG_EXTERNAL_MAPPABLE (1 << 3 | TTM_PAGE_FLAG_EXTERNAL) #define TTM_PAGE_FLAG_PRIV_POPULATED (1 << 31) uint32_t page_flags;
In commit: commit 667a50db0477d47fdff01c666f5ee1ce26b5264c Author: Thomas Hellstrom <thellstrom@vmware.com> Date: Fri Jan 3 11:17:18 2014 +0100 drm/ttm: Refuse to fault (prime-) imported pages we introduced the restriction that imported pages should not be directly mappable through TTM(this also extends to userptr). In the next patch we want to introduce a shmem_tt backend, which should follow all the existing rules with TTM_PAGE_FLAG_EXTERNAL, since it will need to handle swapping itself, but with the above mapping restriction lifted. Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Cc: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/ttm/ttm_bo_vm.c | 6 ++++-- include/drm/ttm/ttm_tt.h | 7 +++++++ 2 files changed, 11 insertions(+), 2 deletions(-)