Re: [PATCH 6/8] drm/vram-helper: don't use ttm bo->offset v3
Am 05.03.20 um 15:35 schrieb Nirmoy: On 3/5/20 3:07 PM, Gerd Hoffmann wrote: On Thu, Mar 05, 2020 at 02:29:08PM +0100, Nirmoy Das wrote: Calculate GEM VRAM bo's offset within vram-helper without depending on bo->offset. Signed-off-by: Nirmoy Das Reviewed-by: Daniel Vetter --- drivers/gpu/drm/drm_gem_vram_helper.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 92a11bb42365..2749c2d25ac4 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -198,6 +198,13 @@ u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo) } EXPORT_SYMBOL(drm_gem_vram_mmap_offset); +static s64 drm_gem_vram_pg_offset(struct drm_gem_vram_object *gbo) +{ + if (WARN_ON_ONCE(!gbo->bo.mem.mm_node)) + return 0; returns 0 on error. I am not sure if we should call this an error. This patch series removes below offset calculation from ttm_bo.c. - if (bo->mem.mm_node) - bo->offset = (bo->mem.start << PAGE_SHIFT) + - bdev->man[bo->mem.mem_type].gpu_offset; - else - bo->offset = 0; - Most of the driver sets "bo->mem.mm_node". Thomas suggested to use this "return 0" in case some driver depends on bo->offset = 0. We should probably add a code comment here to explain why we do this. Something like "Keep TTM behavior for now, remove when drivers are audited". Regards, Christian. + return gbo->bo.mem.start; +} + /** * drm_gem_vram_offset() - \ Returns a GEM VRAM object's offset in video memory @@ -214,7 +221,7 @@ s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo) { if (WARN_ON_ONCE(!gbo->pin_count)) return (s64)-ENODEV; returns -errno on error. - return gbo->bo.offset; + return drm_gem_vram_pg_offset(gbo) << PAGE_SHIFT; And given that one calls the other behavior on error should better be consistent ... cheers, Gerd Regards, Nirmoy ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 6/8] drm/vram-helper: don't use ttm bo->offset v3
On 3/5/20 3:07 PM, Gerd Hoffmann wrote: On Thu, Mar 05, 2020 at 02:29:08PM +0100, Nirmoy Das wrote: Calculate GEM VRAM bo's offset within vram-helper without depending on bo->offset. Signed-off-by: Nirmoy Das Reviewed-by: Daniel Vetter --- drivers/gpu/drm/drm_gem_vram_helper.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 92a11bb42365..2749c2d25ac4 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -198,6 +198,13 @@ u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo) } EXPORT_SYMBOL(drm_gem_vram_mmap_offset); +static s64 drm_gem_vram_pg_offset(struct drm_gem_vram_object *gbo) +{ + if (WARN_ON_ONCE(!gbo->bo.mem.mm_node)) + return 0; returns 0 on error. I am not sure if we should call this an error. This patch series removes below offset calculation from ttm_bo.c. - if (bo->mem.mm_node) - bo->offset = (bo->mem.start << PAGE_SHIFT) + - bdev->man[bo->mem.mem_type].gpu_offset; - else - bo->offset = 0; - Most of the driver sets "bo->mem.mm_node". Thomas suggested to use this "return 0" in case some driver depends on bo->offset = 0. + return gbo->bo.mem.start; +} + /** * drm_gem_vram_offset() - \ Returns a GEM VRAM object's offset in video memory @@ -214,7 +221,7 @@ s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo) { if (WARN_ON_ONCE(!gbo->pin_count)) return (s64)-ENODEV; returns -errno on error. - return gbo->bo.offset; + return drm_gem_vram_pg_offset(gbo) << PAGE_SHIFT; And given that one calls the other behavior on error should better be consistent ... cheers, Gerd Regards, Nirmoy ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 6/8] drm/vram-helper: don't use ttm bo->offset v3
Hi Am 05.03.20 um 15:07 schrieb Gerd Hoffmann: > On Thu, Mar 05, 2020 at 02:29:08PM +0100, Nirmoy Das wrote: >> Calculate GEM VRAM bo's offset within vram-helper without depending on >> bo->offset. >> >> Signed-off-by: Nirmoy Das >> Reviewed-by: Daniel Vetter >> --- >> drivers/gpu/drm/drm_gem_vram_helper.c | 9 - >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c >> b/drivers/gpu/drm/drm_gem_vram_helper.c >> index 92a11bb42365..2749c2d25ac4 100644 >> --- a/drivers/gpu/drm/drm_gem_vram_helper.c >> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c >> @@ -198,6 +198,13 @@ u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object >> *gbo) >> } >> EXPORT_SYMBOL(drm_gem_vram_mmap_offset); >> >> +static s64 drm_gem_vram_pg_offset(struct drm_gem_vram_object *gbo) >> +{ >> +if (WARN_ON_ONCE(!gbo->bo.mem.mm_node)) >> +return 0; > > returns 0 on error. > >> +return gbo->bo.mem.start; >> +} >> + >> /** >> * drm_gem_vram_offset() - \ >> Returns a GEM VRAM object's offset in video memory >> @@ -214,7 +221,7 @@ s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo) >> { >> if (WARN_ON_ONCE(!gbo->pin_count)) >> return (s64)-ENODEV; > > returns -errno on error. > >> -return gbo->bo.offset; >> +return drm_gem_vram_pg_offset(gbo) << PAGE_SHIFT; > > And given that one calls the other behavior on error should better be > consistent ... It is expected that the offset is valid if pin_count is positive. Anything else would be a massive ref-counting bug. And that's been the behavior of the old code as well. But I agree that the current patch is inconsistent. I suggest changing the return type of drm_gem_vram_pg_offset() to u64. Best regards Thomas > > cheers, > Gerd > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer signature.asc Description: OpenPGP digital signature ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 6/8] drm/vram-helper: don't use ttm bo->offset v3
On Thu, Mar 05, 2020 at 02:29:08PM +0100, Nirmoy Das wrote: > Calculate GEM VRAM bo's offset within vram-helper without depending on > bo->offset. > > Signed-off-by: Nirmoy Das > Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/drm_gem_vram_helper.c | 9 - > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c > b/drivers/gpu/drm/drm_gem_vram_helper.c > index 92a11bb42365..2749c2d25ac4 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -198,6 +198,13 @@ u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object > *gbo) > } > EXPORT_SYMBOL(drm_gem_vram_mmap_offset); > > +static s64 drm_gem_vram_pg_offset(struct drm_gem_vram_object *gbo) > +{ > + if (WARN_ON_ONCE(!gbo->bo.mem.mm_node)) > + return 0; returns 0 on error. > + return gbo->bo.mem.start; > +} > + > /** > * drm_gem_vram_offset() - \ > Returns a GEM VRAM object's offset in video memory > @@ -214,7 +221,7 @@ s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo) > { > if (WARN_ON_ONCE(!gbo->pin_count)) > return (s64)-ENODEV; returns -errno on error. > - return gbo->bo.offset; > + return drm_gem_vram_pg_offset(gbo) << PAGE_SHIFT; And given that one calls the other behavior on error should better be consistent ... cheers, Gerd ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 6/8] drm/vram-helper: don't use ttm bo->offset v3
Calculate GEM VRAM bo's offset within vram-helper without depending on bo->offset. Signed-off-by: Nirmoy Das Reviewed-by: Daniel Vetter --- drivers/gpu/drm/drm_gem_vram_helper.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 92a11bb42365..2749c2d25ac4 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -198,6 +198,13 @@ u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo) } EXPORT_SYMBOL(drm_gem_vram_mmap_offset); +static s64 drm_gem_vram_pg_offset(struct drm_gem_vram_object *gbo) +{ + if (WARN_ON_ONCE(!gbo->bo.mem.mm_node)) + return 0; + return gbo->bo.mem.start; +} + /** * drm_gem_vram_offset() - \ Returns a GEM VRAM object's offset in video memory @@ -214,7 +221,7 @@ s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo) { if (WARN_ON_ONCE(!gbo->pin_count)) return (s64)-ENODEV; - return gbo->bo.offset; + return drm_gem_vram_pg_offset(gbo) << PAGE_SHIFT; } EXPORT_SYMBOL(drm_gem_vram_offset); -- 2.25.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 6/8] drm/vram-helper: don't use ttm bo->offset
On Mon, Feb 17, 2020 at 04:04:25PM +0100, Nirmoy Das wrote: > Calculate GPU offset within vram-helper without depending on > bo->offset > > Signed-off-by: Nirmoy Das Acked-by: Gerd Hoffmann ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 6/8] drm/vram-helper: don't use ttm bo->offset v2
Hi Am 19.02.20 um 14:53 schrieb Nirmoy Das: > Calculate GEM VRAM bo's offset within vram-helper without depending on > bo->offset > > Signed-off-by: Nirmoy Das > --- > drivers/gpu/drm/drm_gem_vram_helper.c | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c > b/drivers/gpu/drm/drm_gem_vram_helper.c > index 92a11bb42365..3edf5f241c15 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -198,6 +198,21 @@ u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object > *gbo) > } > EXPORT_SYMBOL(drm_gem_vram_mmap_offset); > > +/** > + * drm_gem_vram_pg_offset() - Returns a GEM VRAM object's page offset > + * @gbo: the GEM VRAM object > + * > + * Returns: > + * The buffer object's page offset, or > + * 0 with a warning when memory manager node of the buffer object is NULL > + * */ > +static s64 drm_gem_vram_pg_offset(struct drm_gem_vram_object *gbo) > +{ > + if (WARN_ON_ONCE(!gbo->bo.mem.mm_node)) > + return 0; > + return gbo->bo.mem.start; > +} As Daniel said, you don't heve to document this function. Otherwise Reviewed-by: Thomas Zimmermann > + > /** > * drm_gem_vram_offset() - \ > Returns a GEM VRAM object's offset in video memory > @@ -214,7 +229,7 @@ s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo) > { > if (WARN_ON_ONCE(!gbo->pin_count)) > return (s64)-ENODEV; > - return gbo->bo.offset; > + return drm_gem_vram_pg_offset(gbo) << PAGE_SHIFT; > } > EXPORT_SYMBOL(drm_gem_vram_offset); > > -- > 2.25.0 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer signature.asc Description: OpenPGP digital signature ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 6/8] drm/vram-helper: don't use ttm bo->offset v2
On 2/20/20 7:09 PM, Daniel Vetter wrote: On Wed, Feb 19, 2020 at 02:53:20PM +0100, Nirmoy Das wrote: Calculate GEM VRAM bo's offset within vram-helper without depending on bo->offset Signed-off-by: Nirmoy Das --- drivers/gpu/drm/drm_gem_vram_helper.c | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 92a11bb42365..3edf5f241c15 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -198,6 +198,21 @@ u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo) } EXPORT_SYMBOL(drm_gem_vram_mmap_offset); +/** + * drm_gem_vram_pg_offset() - Returns a GEM VRAM object's page offset + * @gbo: the GEM VRAM object + * + * Returns: + * The buffer object's page offset, or + * 0 with a warning when memory manager node of the buffer object is NULL + * */ We generally don't add full formal kerneldoc for internal functions like this. It won't get pulled into generated docs and generally just bitrots. Just informal comment if it's really tricky, but the function name here is clear enough I think. Thanks for you review Daniel, I will remove that comment. Nirmoy So with the comment removed: Reviewed-by: Daniel Vetter +static s64 drm_gem_vram_pg_offset(struct drm_gem_vram_object *gbo) +{ + if (WARN_ON_ONCE(!gbo->bo.mem.mm_node)) + return 0; + return gbo->bo.mem.start; +} + /** * drm_gem_vram_offset() - \ Returns a GEM VRAM object's offset in video memory @@ -214,7 +229,7 @@ s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo) { if (WARN_ON_ONCE(!gbo->pin_count)) return (s64)-ENODEV; - return gbo->bo.offset; + return drm_gem_vram_pg_offset(gbo) << PAGE_SHIFT; } EXPORT_SYMBOL(drm_gem_vram_offset); -- 2.25.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 6/8] drm/vram-helper: don't use ttm bo->offset v2
On Wed, Feb 19, 2020 at 02:53:20PM +0100, Nirmoy Das wrote: > Calculate GEM VRAM bo's offset within vram-helper without depending on > bo->offset > > Signed-off-by: Nirmoy Das > --- > drivers/gpu/drm/drm_gem_vram_helper.c | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c > b/drivers/gpu/drm/drm_gem_vram_helper.c > index 92a11bb42365..3edf5f241c15 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -198,6 +198,21 @@ u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object > *gbo) > } > EXPORT_SYMBOL(drm_gem_vram_mmap_offset); > > +/** > + * drm_gem_vram_pg_offset() - Returns a GEM VRAM object's page offset > + * @gbo: the GEM VRAM object > + * > + * Returns: > + * The buffer object's page offset, or > + * 0 with a warning when memory manager node of the buffer object is NULL > + * */ We generally don't add full formal kerneldoc for internal functions like this. It won't get pulled into generated docs and generally just bitrots. Just informal comment if it's really tricky, but the function name here is clear enough I think. So with the comment removed: Reviewed-by: Daniel Vetter > +static s64 drm_gem_vram_pg_offset(struct drm_gem_vram_object *gbo) > +{ > + if (WARN_ON_ONCE(!gbo->bo.mem.mm_node)) > + return 0; > + return gbo->bo.mem.start; > +} > + > /** > * drm_gem_vram_offset() - \ > Returns a GEM VRAM object's offset in video memory > @@ -214,7 +229,7 @@ s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo) > { > if (WARN_ON_ONCE(!gbo->pin_count)) > return (s64)-ENODEV; > - return gbo->bo.offset; > + return drm_gem_vram_pg_offset(gbo) << PAGE_SHIFT; > } > EXPORT_SYMBOL(drm_gem_vram_offset); > > -- > 2.25.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 6/8] drm/vram-helper: don't use ttm bo->offset v2
Calculate GEM VRAM bo's offset within vram-helper without depending on bo->offset Signed-off-by: Nirmoy Das --- drivers/gpu/drm/drm_gem_vram_helper.c | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 92a11bb42365..3edf5f241c15 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -198,6 +198,21 @@ u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo) } EXPORT_SYMBOL(drm_gem_vram_mmap_offset); +/** + * drm_gem_vram_pg_offset() - Returns a GEM VRAM object's page offset + * @gbo: the GEM VRAM object + * + * Returns: + * The buffer object's page offset, or + * 0 with a warning when memory manager node of the buffer object is NULL + * */ +static s64 drm_gem_vram_pg_offset(struct drm_gem_vram_object *gbo) +{ + if (WARN_ON_ONCE(!gbo->bo.mem.mm_node)) + return 0; + return gbo->bo.mem.start; +} + /** * drm_gem_vram_offset() - \ Returns a GEM VRAM object's offset in video memory @@ -214,7 +229,7 @@ s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo) { if (WARN_ON_ONCE(!gbo->pin_count)) return (s64)-ENODEV; - return gbo->bo.offset; + return drm_gem_vram_pg_offset(gbo) << PAGE_SHIFT; } EXPORT_SYMBOL(drm_gem_vram_offset); -- 2.25.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 6/8] drm/vram-helper: don't use ttm bo->offset
Calculate GPU offset within vram-helper without depending on bo->offset Signed-off-by: Nirmoy Das --- drivers/gpu/drm/drm_gem_vram_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 92a11bb42365..e7ef4cd8116d 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -214,7 +214,7 @@ s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo) { if (WARN_ON_ONCE(!gbo->pin_count)) return (s64)-ENODEV; - return gbo->bo.offset; + return gbo->bo.mem.start << PAGE_SHIFT; } EXPORT_SYMBOL(drm_gem_vram_offset); -- 2.25.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx