| Message ID | 20210728130552.2074-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/vmwgfx: unbind in vmw_ttm_unpopulate | expand |
Adding Thomas on CC as well. Just a gentle ping. I think the patch set makes sense now. Regards, Christian. Am 28.07.21 um 15:05 schrieb Christian König: > Doing this in vmw_ttm_destroy() is to late. > > It turned out that this is not a good idea at all because it leaves pointers > to freed up system memory pages in the GART tables of the drivers. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > index b0973c27e774..904031d03dbe 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c > @@ -526,14 +526,9 @@ static void vmw_ttm_destroy(struct ttm_device *bdev, struct ttm_tt *ttm) > struct vmw_ttm_tt *vmw_be = > container_of(ttm, struct vmw_ttm_tt, dma_ttm); > > - vmw_ttm_unbind(bdev, ttm); > ttm_tt_destroy_common(bdev, ttm); > vmw_ttm_unmap_dma(vmw_be); > - if (vmw_be->dev_priv->map_mode == vmw_dma_alloc_coherent) > - ttm_tt_fini(&vmw_be->dma_ttm); > - else > - ttm_tt_fini(ttm); > - > + ttm_tt_fini(ttm); > if (vmw_be->mob) > vmw_mob_destroy(vmw_be->mob); > > @@ -578,6 +573,8 @@ static void vmw_ttm_unpopulate(struct ttm_device *bdev, > dma_ttm); > unsigned int i; > > + vmw_ttm_unbind(bdev, ttm); > + > if (vmw_tt->mob) { > vmw_mob_destroy(vmw_tt->mob); > vmw_tt->mob = NULL;
On Mon, 2021-08-23 at 13:05 +0200, Christian König wrote: > Adding Thomas on CC as well. > > Just a gentle ping. I think the patch set makes sense now. > > Regards, > Christian. > > Am 28.07.21 um 15:05 schrieb Christian König: > > Doing this in vmw_ttm_destroy() is to late. > > > > It turned out that this is not a good idea at all because it leaves > > pointers > > to freed up system memory pages in the GART tables of the drivers. > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > --- > > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 9 +++------ > > 1 file changed, 3 insertions(+), 6 deletions(-) > > Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
On Mon, Aug 23, 2021 at 01:15:20PM +0200, Thomas Hellström wrote: > On Mon, 2021-08-23 at 13:05 +0200, Christian König wrote: > > Adding Thomas on CC as well. > > > > Just a gentle ping. I think the patch set makes sense now. > > > > Regards, > > Christian. > > > > Am 28.07.21 um 15:05 schrieb Christian König: > > > Doing this in vmw_ttm_destroy() is to late. > > > > > > It turned out that this is not a good idea at all because it leaves > > > pointers > > > to freed up system memory pages in the GART tables of the drivers. > > > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > > --- > > > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 9 +++------ > > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > > Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> For next time around I think recording a bit more of the discussions and git history in these would be really good. At least I'd like to get more people ramped up on ttm work, and for that to work out things need to be a bit more accessible ... The above commit message is pretty much useless if you ever hit it in a git blame, if you haven't been involved in any of these discussions. -Daniel
Am 26.08.21 um 10:49 schrieb Daniel Vetter: > On Mon, Aug 23, 2021 at 01:15:20PM +0200, Thomas Hellström wrote: >> On Mon, 2021-08-23 at 13:05 +0200, Christian König wrote: >>> Adding Thomas on CC as well. >>> >>> Just a gentle ping. I think the patch set makes sense now. >>> >>> Regards, >>> Christian. >>> >>> Am 28.07.21 um 15:05 schrieb Christian König: >>>> Doing this in vmw_ttm_destroy() is to late. >>>> >>>> It turned out that this is not a good idea at all because it leaves >>>> pointers >>>> to freed up system memory pages in the GART tables of the drivers. >>>> >>>> Signed-off-by: Christian König <christian.koenig@amd.com> >>>> --- >>>> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 9 +++------ >>>> 1 file changed, 3 insertions(+), 6 deletions(-) >>>> >> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > For next time around I think recording a bit more of the discussions and > git history in these would be really good. At least I'd like to get more > people ramped up on ttm work, and for that to work out things need to be a > bit more accessible ... The above commit message is pretty much useless if > you ever hit it in a git blame, if you haven't been involved in any of > these discussions. I've pushed it with a link tag back to the patches in patchwork, but I should probably include a link tag to the older versions as well for completeness. Christian. > -Daniel
On Thu, Aug 26, 2021 at 12:11:04PM +0200, Christian König wrote: > > > Am 26.08.21 um 10:49 schrieb Daniel Vetter: > > On Mon, Aug 23, 2021 at 01:15:20PM +0200, Thomas Hellström wrote: > > > On Mon, 2021-08-23 at 13:05 +0200, Christian König wrote: > > > > Adding Thomas on CC as well. > > > > > > > > Just a gentle ping. I think the patch set makes sense now. > > > > > > > > Regards, > > > > Christian. > > > > > > > > Am 28.07.21 um 15:05 schrieb Christian König: > > > > > Doing this in vmw_ttm_destroy() is to late. > > > > > > > > > > It turned out that this is not a good idea at all because it leaves > > > > > pointers > > > > > to freed up system memory pages in the GART tables of the drivers. > > > > > > > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > > > > --- > > > > > drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 9 +++------ > > > > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > > > > > Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > For next time around I think recording a bit more of the discussions and > > git history in these would be really good. At least I'd like to get more > > people ramped up on ttm work, and for that to work out things need to be a > > bit more accessible ... The above commit message is pretty much useless if > > you ever hit it in a git blame, if you haven't been involved in any of > > these discussions. > > I've pushed it with a link tag back to the patches in patchwork, but I > should probably include a link tag to the older versions as well for > completeness. Imo references to the commits that landed which broke stuff and full quotes of the relevant discussions in the other thread. Just adding the links still means you have to painstakingly stitch the story together yourself, but at least it would be somewhat possible. Really I think stuff like this should be documented in kerneldoc for these callbacks, and we just got to start doing that. Waiting until magically someone else fixes the ttm kerneldoc wont happen :-) -Daniel
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c index b0973c27e774..904031d03dbe 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c @@ -526,14 +526,9 @@ static void vmw_ttm_destroy(struct ttm_device *bdev, struct ttm_tt *ttm) struct vmw_ttm_tt *vmw_be = container_of(ttm, struct vmw_ttm_tt, dma_ttm); - vmw_ttm_unbind(bdev, ttm); ttm_tt_destroy_common(bdev, ttm); vmw_ttm_unmap_dma(vmw_be); - if (vmw_be->dev_priv->map_mode == vmw_dma_alloc_coherent) - ttm_tt_fini(&vmw_be->dma_ttm); - else - ttm_tt_fini(ttm); - + ttm_tt_fini(ttm); if (vmw_be->mob) vmw_mob_destroy(vmw_be->mob); @@ -578,6 +573,8 @@ static void vmw_ttm_unpopulate(struct ttm_device *bdev, dma_ttm); unsigned int i; + vmw_ttm_unbind(bdev, ttm); + if (vmw_tt->mob) { vmw_mob_destroy(vmw_tt->mob); vmw_tt->mob = NULL;
Doing this in vmw_ttm_destroy() is to late. It turned out that this is not a good idea at all because it leaves pointers to freed up system memory pages in the GART tables of the drivers. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)