| Message ID | 20201008112342.9394-2-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] mm: introduce vma_set_file function v2 | expand |
On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote: > We have reoccurring requests on this so better document that > this approach doesn't work and dma_buf_mmap() needs to be used instead. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/drm_prime.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 4910c446db83..16fa2bfc271e 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import); > /** > * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array > * @sgt: scatter-gather table to convert > - * @pages: optional array of page pointers to store the page array in > + * @pages: deprecated array of page pointers to store the page array in > * @addrs: optional array to store the dma bus address of each page > * @max_entries: size of both the passed-in arrays > * > @@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import); > * > * Drivers can use this in their &drm_driver.gem_prime_import_sg_table > * implementation. > + * > + * Specifying the pages array is deprecated and strongly discouraged for new > + * drivers. The pages array is only useful for page faults and those can > + * corrupt fields in the struct page if they are not handled by the exporting > + * driver. > */ I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers using this only need it for the pages array. Imo just open-code the sg table walking loop in amdgpu/radeon (it's really not much code), and then drop the dma_addr_t parameter from this function here (it's set to NULL by everyone else). And then deprecate this entire function here with a big warning that a) dma_buf_map_attachment is allowed to leave the struct page pointers NULL and b) this breaks mmap, users must call dma_buf_mmap instead. Also maybe make it an uppercase DEPRECATED or something like that :-) -Daniel > int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, > dma_addr_t *addrs, int max_entries) > -- > 2.17.1 >
On Thu, Oct 08, 2020 at 04:09:14PM +0200, Daniel Vetter wrote: > On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote: > > We have reoccurring requests on this so better document that > > this approach doesn't work and dma_buf_mmap() needs to be used instead. > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > --- > > drivers/gpu/drm/drm_prime.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > > index 4910c446db83..16fa2bfc271e 100644 > > --- a/drivers/gpu/drm/drm_prime.c > > +++ b/drivers/gpu/drm/drm_prime.c > > @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import); > > /** > > * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array > > * @sgt: scatter-gather table to convert > > - * @pages: optional array of page pointers to store the page array in > > + * @pages: deprecated array of page pointers to store the page array in > > * @addrs: optional array to store the dma bus address of each page > > * @max_entries: size of both the passed-in arrays > > * > > @@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import); > > * > > * Drivers can use this in their &drm_driver.gem_prime_import_sg_table > > * implementation. > > + * > > + * Specifying the pages array is deprecated and strongly discouraged for new > > + * drivers. The pages array is only useful for page faults and those can > > + * corrupt fields in the struct page if they are not handled by the exporting > > + * driver. > > */ > > I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers > using this only need it for the pages array. Imo just open-code the sg > table walking loop in amdgpu/radeon (it's really not much code), and then > drop the dma_addr_t parameter from this function here (it's set to NULL by > everyone else). > > And then deprecate this entire function here with a big warning that a) > dma_buf_map_attachment is allowed to leave the struct page pointers NULL > and b) this breaks mmap, users must call dma_buf_mmap instead. > > Also maybe make it an uppercase DEPRECATED or something like that :-) OK I just realized I missed nouveau. That would be 3 places where we need to stuff the dma_addr_t list into something ttm can take. Still feels better than this half-deprecated function kludge ... -Daniel > -Daniel > > > int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, > > dma_addr_t *addrs, int max_entries) > > -- > > 2.17.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Am 08.10.20 um 16:14 schrieb Daniel Vetter: > On Thu, Oct 08, 2020 at 04:09:14PM +0200, Daniel Vetter wrote: >> On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote: >>> We have reoccurring requests on this so better document that >>> this approach doesn't work and dma_buf_mmap() needs to be used instead. >>> >>> Signed-off-by: Christian König <christian.koenig@amd.com> >>> --- >>> drivers/gpu/drm/drm_prime.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >>> index 4910c446db83..16fa2bfc271e 100644 >>> --- a/drivers/gpu/drm/drm_prime.c >>> +++ b/drivers/gpu/drm/drm_prime.c >>> @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import); >>> /** >>> * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array >>> * @sgt: scatter-gather table to convert >>> - * @pages: optional array of page pointers to store the page array in >>> + * @pages: deprecated array of page pointers to store the page array in >>> * @addrs: optional array to store the dma bus address of each page >>> * @max_entries: size of both the passed-in arrays >>> * >>> @@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import); >>> * >>> * Drivers can use this in their &drm_driver.gem_prime_import_sg_table >>> * implementation. >>> + * >>> + * Specifying the pages array is deprecated and strongly discouraged for new >>> + * drivers. The pages array is only useful for page faults and those can >>> + * corrupt fields in the struct page if they are not handled by the exporting >>> + * driver. >>> */ >> I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers >> using this only need it for the pages array. Imo just open-code the sg >> table walking loop in amdgpu/radeon (it's really not much code), and then >> drop the dma_addr_t parameter from this function here (it's set to NULL by >> everyone else). >> >> And then deprecate this entire function here with a big warning that a) >> dma_buf_map_attachment is allowed to leave the struct page pointers NULL >> and b) this breaks mmap, users must call dma_buf_mmap instead. >> >> Also maybe make it an uppercase DEPRECATED or something like that :-) > OK I just realized I missed nouveau. That would be 3 places where we need > to stuff the dma_addr_t list into something ttm can take. Still feels > better than this half-deprecated function kludge ... Mhm, I don't see a reason why nouveau would need the struct page either. How about we split that up into two function? One for converting the sg_table into a linear dma_addr array. And one for converting the sg_table into a linear struct page array with a __deprecated attribute on it? Christian. > -Daniel > >> -Daniel >> >>> int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, >>> dma_addr_t *addrs, int max_entries) >>> -- >>> 2.17.1 >>> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch
On Fri, Oct 09, 2020 at 09:36:41AM +0200, Christian König wrote: > Am 08.10.20 um 16:14 schrieb Daniel Vetter: > > On Thu, Oct 08, 2020 at 04:09:14PM +0200, Daniel Vetter wrote: > > > On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote: > > > > We have reoccurring requests on this so better document that > > > > this approach doesn't work and dma_buf_mmap() needs to be used instead. > > > > > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > > > --- > > > > drivers/gpu/drm/drm_prime.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > > > > index 4910c446db83..16fa2bfc271e 100644 > > > > --- a/drivers/gpu/drm/drm_prime.c > > > > +++ b/drivers/gpu/drm/drm_prime.c > > > > @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import); > > > > /** > > > > * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array > > > > * @sgt: scatter-gather table to convert > > > > - * @pages: optional array of page pointers to store the page array in > > > > + * @pages: deprecated array of page pointers to store the page array in > > > > * @addrs: optional array to store the dma bus address of each page > > > > * @max_entries: size of both the passed-in arrays > > > > * > > > > @@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import); > > > > * > > > > * Drivers can use this in their &drm_driver.gem_prime_import_sg_table > > > > * implementation. > > > > + * > > > > + * Specifying the pages array is deprecated and strongly discouraged for new > > > > + * drivers. The pages array is only useful for page faults and those can > > > > + * corrupt fields in the struct page if they are not handled by the exporting > > > > + * driver. > > > > */ > > > I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers > > > using this only need it for the pages array. Imo just open-code the sg > > > table walking loop in amdgpu/radeon (it's really not much code), and then > > > drop the dma_addr_t parameter from this function here (it's set to NULL by > > > everyone else). > > > > > > And then deprecate this entire function here with a big warning that a) > > > dma_buf_map_attachment is allowed to leave the struct page pointers NULL > > > and b) this breaks mmap, users must call dma_buf_mmap instead. > > > > > > Also maybe make it an uppercase DEPRECATED or something like that :-) > > OK I just realized I missed nouveau. That would be 3 places where we need > > to stuff the dma_addr_t list into something ttm can take. Still feels > > better than this half-deprecated function kludge ... > > Mhm, I don't see a reason why nouveau would need the struct page either. > > How about we split that up into two function? > > One for converting the sg_table into a linear dma_addr array. > > And one for converting the sg_table into a linear struct page array with a > __deprecated attribute on it? Yeah makes sense, since converting ttm to just use sgt iterations directly everywhere is probably a bit too much. Maybe keep that converter in ttm code, since outside of ttm the rough consensus is to converge on sgt for handling buffers. Well, for those drivers not stuck on struct page arrays :-) -Daniel > > Christian. > > > -Daniel > > > > > -Daniel > > > > > > > int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, > > > > dma_addr_t *addrs, int max_entries) > > > > -- > > > > 2.17.1 > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 4910c446db83..16fa2bfc271e 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import); /** * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array * @sgt: scatter-gather table to convert - * @pages: optional array of page pointers to store the page array in + * @pages: deprecated array of page pointers to store the page array in * @addrs: optional array to store the dma bus address of each page * @max_entries: size of both the passed-in arrays * @@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import); * * Drivers can use this in their &drm_driver.gem_prime_import_sg_table * implementation. + * + * Specifying the pages array is deprecated and strongly discouraged for new + * drivers. The pages array is only useful for page faults and those can + * corrupt fields in the struct page if they are not handled by the exporting + * driver. */ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, dma_addr_t *addrs, int max_entries)
We have reoccurring requests on this so better document that this approach doesn't work and dma_buf_mmap() needs to be used instead. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_prime.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)