| Message ID | fe9f66a7eb918a3e62473e11c29b973d2c5395ce.1523465719.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 11.04.2018 um 19:11 schrieb Robin Murphy: > Now that drm_prime_sg_to_page_addr_arrays() understands the case where > dma_map_sg() has coalesced segments and returns 0 < count < nents, we > can relax the check to only consider genuine failure. That pattern is repeated in pretty much all drivers using TTM. So you would need to fix all of them, but as I said I don't think that this approach is a good idea. We essentially wanted to get rid of the dma_address array in the mid term and that change goes into the exactly opposite direction. Regards, Christian. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 205da3ff9cd0..f81e96a4242f 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -813,7 +813,7 @@ static int amdgpu_ttm_tt_pin_userptr(struct ttm_tt *ttm) > > r = -ENOMEM; > nents = dma_map_sg(adev->dev, ttm->sg->sgl, ttm->sg->nents, direction); > - if (nents != ttm->sg->nents) > + if (nents == 0) > goto release_sg; > > drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
On 11/04/18 19:28, Christian König wrote: > Am 11.04.2018 um 19:11 schrieb Robin Murphy: >> Now that drm_prime_sg_to_page_addr_arrays() understands the case where >> dma_map_sg() has coalesced segments and returns 0 < count < nents, we >> can relax the check to only consider genuine failure. > > That pattern is repeated in pretty much all drivers using TTM. AFAICS from a look through drivers/gpu/ only 3 drivers consider the actual value of what dma_map_sg() returns - others only handle the failure case of 0, and some don't check it at all - and of those it's only amdgpu and radeon barfing on it being different from nents (vmwgfx appears to just stash it to use slightly incorrectly later). > So you would need to fix all of them, but as I said I don't think that > this approach is a good idea. > > We essentially wanted to get rid of the dma_address array in the mid > term and that change goes into the exactly opposite direction. But this patch bears no intention of making any fundamental change to the existing behaviour of this particular driver, it simply permits said behaviour to work at all on more systems than it currently does. I would consider that entirely orthogonal to future TTM-wide development :/ Robin. > > Regards, > Christian. > >> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> index 205da3ff9cd0..f81e96a4242f 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> @@ -813,7 +813,7 @@ static int amdgpu_ttm_tt_pin_userptr(struct ttm_tt >> *ttm) >> r = -ENOMEM; >> nents = dma_map_sg(adev->dev, ttm->sg->sgl, ttm->sg->nents, >> direction); >> - if (nents != ttm->sg->nents) >> + if (nents == 0) >> goto release_sg; >> drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages, >
Am 12.04.2018 um 19:53 schrieb Robin Murphy: > On 11/04/18 19:28, Christian König wrote: >> Am 11.04.2018 um 19:11 schrieb Robin Murphy: >>> Now that drm_prime_sg_to_page_addr_arrays() understands the case where >>> dma_map_sg() has coalesced segments and returns 0 < count < nents, we >>> can relax the check to only consider genuine failure. >> >> That pattern is repeated in pretty much all drivers using TTM. > > AFAICS from a look through drivers/gpu/ only 3 drivers consider the > actual value of what dma_map_sg() returns - others only handle the > failure case of 0, and some don't check it at all - and of those it's > only amdgpu and radeon barfing on it being different from nents > (vmwgfx appears to just stash it to use slightly incorrectly later). > >> So you would need to fix all of them, but as I said I don't think >> that this approach is a good idea. >> >> We essentially wanted to get rid of the dma_address array in the mid >> term and that change goes into the exactly opposite direction. > > But this patch bears no intention of making any fundamental change to > the existing behaviour of this particular driver, it simply permits > said behaviour to work at all on more systems than it currently does. > I would consider that entirely orthogonal to future TTM-wide > development :/ That's a really good point. Please try to fix radeon as well and maybe ping the vmwgfx maintainers. With that in place I'm perfectly ok with going ahead with that approach. Christian. > > Robin. > >> >> Regards, >> Christian. >> >>> >>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> index 205da3ff9cd0..f81e96a4242f 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >>> @@ -813,7 +813,7 @@ static int amdgpu_ttm_tt_pin_userptr(struct >>> ttm_tt *ttm) >>> r = -ENOMEM; >>> nents = dma_map_sg(adev->dev, ttm->sg->sgl, ttm->sg->nents, >>> direction); >>> - if (nents != ttm->sg->nents) >>> + if (nents == 0) >>> goto release_sg; >>> drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages, >>
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 205da3ff9cd0..f81e96a4242f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -813,7 +813,7 @@ static int amdgpu_ttm_tt_pin_userptr(struct ttm_tt *ttm) r = -ENOMEM; nents = dma_map_sg(adev->dev, ttm->sg->sgl, ttm->sg->nents, direction); - if (nents != ttm->sg->nents) + if (nents == 0) goto release_sg; drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
Now that drm_prime_sg_to_page_addr_arrays() understands the case where dma_map_sg() has coalesced segments and returns 0 < count < nents, we can relax the check to only consider genuine failure. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)