The malidp driver uses GEM object functions for callbacks. Fix it to use them internally as well.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Emil Velikov emil.velikov@collabora.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Brian Starkey brian.starkey@arm.com --- drivers/gpu/drm/arm/malidp_planes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c index ab45ac445045..351a85088d0e 100644 --- a/drivers/gpu/drm/arm/malidp_planes.c +++ b/drivers/gpu/drm/arm/malidp_planes.c @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, if (cma_obj->sgt) sgt = cma_obj->sgt; else - sgt = obj->dev->driver->gem_prime_get_sg_table(obj); + sgt = obj->funcs->get_sg_table(obj);
if (!sgt) return false;
On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote:
The malidp driver uses GEM object functions for callbacks. Fix it to use them internally as well.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Emil Velikov emil.velikov@collabora.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Brian Starkey brian.starkey@arm.com
drivers/gpu/drm/arm/malidp_planes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c index ab45ac445045..351a85088d0e 100644 --- a/drivers/gpu/drm/arm/malidp_planes.c +++ b/drivers/gpu/drm/arm/malidp_planes.c @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, if (cma_obj->sgt) sgt = cma_obj->sgt; else
sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
sgt = obj->funcs->get_sg_table(obj);
Uh if there's not a switch somewhere I'd just call the right function directly. Or call the right wrapper for this, this feels a bit fishy ... -Daniel
if (!sgt) return false;-- 2.28.0
Hi
Am 07.08.20 um 15:12 schrieb Daniel Vetter:
On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote:
The malidp driver uses GEM object functions for callbacks. Fix it to use them internally as well.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Emil Velikov emil.velikov@collabora.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Brian Starkey brian.starkey@arm.com
drivers/gpu/drm/arm/malidp_planes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c index ab45ac445045..351a85088d0e 100644 --- a/drivers/gpu/drm/arm/malidp_planes.c +++ b/drivers/gpu/drm/arm/malidp_planes.c @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, if (cma_obj->sgt) sgt = cma_obj->sgt; else
sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
sgt = obj->funcs->get_sg_table(obj);Uh if there's not a switch somewhere I'd just call the right function directly. Or call the right wrapper for this, this feels a bit fishy ...
The driver initializes the pointer via CMA helper macro to an CMA-internal default. Calling the actual function here is fragile if the CMA-internal default ever changes.
But I have no strong feelings. I'll go with whatever the driver's maintainer prefers.
Best regards Thomas
-Daniel
if (!sgt) return false;-- 2.28.0
On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 07.08.20 um 15:12 schrieb Daniel Vetter:
On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote:
The malidp driver uses GEM object functions for callbacks. Fix it to use them internally as well.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Emil Velikov emil.velikov@collabora.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Brian Starkey brian.starkey@arm.com
drivers/gpu/drm/arm/malidp_planes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c index ab45ac445045..351a85088d0e 100644 --- a/drivers/gpu/drm/arm/malidp_planes.c +++ b/drivers/gpu/drm/arm/malidp_planes.c @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, if (cma_obj->sgt) sgt = cma_obj->sgt; else
sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
sgt = obj->funcs->get_sg_table(obj);Uh if there's not a switch somewhere I'd just call the right function directly. Or call the right wrapper for this, this feels a bit fishy ...
The driver initializes the pointer via CMA helper macro to an CMA-internal default. Calling the actual function here is fragile if the CMA-internal default ever changes.
But I have no strong feelings. I'll go with whatever the driver's maintainer prefers.
What I meant is: There should be an exported helper to get at the sgt. Drivers using helpers shouldn't need to do this kind of stuff here.
Also the entire code is fairly suspect, getting at the sgt from plane_check is a bit iffy. But that's a different kind of problem. -Daniel
Hi
Am 07.08.20 um 18:10 schrieb Daniel Vetter:
On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 07.08.20 um 15:12 schrieb Daniel Vetter:
On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote:
The malidp driver uses GEM object functions for callbacks. Fix it to use them internally as well.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Emil Velikov emil.velikov@collabora.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Brian Starkey brian.starkey@arm.com
drivers/gpu/drm/arm/malidp_planes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c index ab45ac445045..351a85088d0e 100644 --- a/drivers/gpu/drm/arm/malidp_planes.c +++ b/drivers/gpu/drm/arm/malidp_planes.c @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, if (cma_obj->sgt) sgt = cma_obj->sgt; else
sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
sgt = obj->funcs->get_sg_table(obj);Uh if there's not a switch somewhere I'd just call the right function directly. Or call the right wrapper for this, this feels a bit fishy ...
The driver initializes the pointer via CMA helper macro to an CMA-internal default. Calling the actual function here is fragile if the CMA-internal default ever changes.
But I have no strong feelings. I'll go with whatever the driver's maintainer prefers.
What I meant is: There should be an exported helper to get at the sgt. Drivers using helpers shouldn't need to do this kind of stuff here.
Also the entire code is fairly suspect, getting at the sgt from plane_check is a bit iffy. But that's a different kind of problem.
I tried to somehow move the code to CMA, but it's not easy. There's no good place to put the look-up code of sgt. And sgt is later being freed iff it came from the callback (and not freed if it was stored in the object). AFIACT the best options are to either keep the code here or move the entire function to CMA helpers.
Best regards Thomas
-Daniel
On Thu, Aug 13, 2020 at 11:19:31AM +0200, Thomas Zimmermann wrote:
Hi
Am 07.08.20 um 18:10 schrieb Daniel Vetter:
On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 07.08.20 um 15:12 schrieb Daniel Vetter:
On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote:
The malidp driver uses GEM object functions for callbacks. Fix it to use them internally as well.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Emil Velikov emil.velikov@collabora.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Brian Starkey brian.starkey@arm.com
drivers/gpu/drm/arm/malidp_planes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c index ab45ac445045..351a85088d0e 100644 --- a/drivers/gpu/drm/arm/malidp_planes.c +++ b/drivers/gpu/drm/arm/malidp_planes.c @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, if (cma_obj->sgt) sgt = cma_obj->sgt; else
sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
sgt = obj->funcs->get_sg_table(obj);Uh if there's not a switch somewhere I'd just call the right function directly. Or call the right wrapper for this, this feels a bit fishy ...
The driver initializes the pointer via CMA helper macro to an CMA-internal default. Calling the actual function here is fragile if the CMA-internal default ever changes.
But I have no strong feelings. I'll go with whatever the driver's maintainer prefers.
What I meant is: There should be an exported helper to get at the sgt. Drivers using helpers shouldn't need to do this kind of stuff here.
Also the entire code is fairly suspect, getting at the sgt from plane_check is a bit iffy. But that's a different kind of problem.
I tried to somehow move the code to CMA, but it's not easy. There's no good place to put the look-up code of sgt. And sgt is later being freed iff it came from the callback (and not freed if it was stored in the object). AFIACT the best options are to either keep the code here or move the entire function to CMA helpers.
Ok I read some code ... I'm confused. From the control flow it looks like malidp is using cma helpers. Otherwise why does the upcasting not blow up sometimes.
But cma helpers already check at import time that any imported dma-buf is contiguous, and they guarantee to fill out the cma_obj->sgt.
So really no idea what this code is doing here.
It's also not correct, since it doesn't coalesce sgt entries, since a range might be split up, but still mapped into a contiguous dma_addr_t range when you take it all together. The code in drm_gem_cma_prime_import_sg_table() gets this more right.
So maybe right fix is to just ditch this all, and use cma helpers fully? -Daniel
Best regards Thomas
-Daniel
-- 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
Am 13.08.20 um 11:48 schrieb Daniel Vetter:
On Thu, Aug 13, 2020 at 11:19:31AM +0200, Thomas Zimmermann wrote:
Hi
Am 07.08.20 um 18:10 schrieb Daniel Vetter:
On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 07.08.20 um 15:12 schrieb Daniel Vetter:
On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote:
The malidp driver uses GEM object functions for callbacks. Fix it to use them internally as well.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Emil Velikov emil.velikov@collabora.com Cc: Liviu Dudau liviu.dudau@arm.com Cc: Brian Starkey brian.starkey@arm.com
drivers/gpu/drm/arm/malidp_planes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c index ab45ac445045..351a85088d0e 100644 --- a/drivers/gpu/drm/arm/malidp_planes.c +++ b/drivers/gpu/drm/arm/malidp_planes.c @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, if (cma_obj->sgt) sgt = cma_obj->sgt; else
sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
sgt = obj->funcs->get_sg_table(obj);Uh if there's not a switch somewhere I'd just call the right function directly. Or call the right wrapper for this, this feels a bit fishy ...
The driver initializes the pointer via CMA helper macro to an CMA-internal default. Calling the actual function here is fragile if the CMA-internal default ever changes.
But I have no strong feelings. I'll go with whatever the driver's maintainer prefers.
What I meant is: There should be an exported helper to get at the sgt. Drivers using helpers shouldn't need to do this kind of stuff here.
Also the entire code is fairly suspect, getting at the sgt from plane_check is a bit iffy. But that's a different kind of problem.
I tried to somehow move the code to CMA, but it's not easy. There's no good place to put the look-up code of sgt. And sgt is later being freed iff it came from the callback (and not freed if it was stored in the object). AFIACT the best options are to either keep the code here or move the entire function to CMA helpers.
Ok I read some code ... I'm confused. From the control flow it looks like malidp is using cma helpers. Otherwise why does the upcasting not blow up sometimes.
But cma helpers already check at import time that any imported dma-buf is contiguous, and they guarantee to fill out the cma_obj->sgt.
So really no idea what this code is doing here.
It's also not correct, since it doesn't coalesce sgt entries, since a range might be split up, but still mapped into a contiguous dma_addr_t range when you take it all together. The code in drm_gem_cma_prime_import_sg_table() gets this more right.
So maybe right fix is to just ditch this all, and use cma helpers fully?
The driver already does use CMA, including drm_gem_cma_prime_import_sg_table().
The patched code is not about importing/exporting sg tables. It configures the MMU's prefetching pattern by looking at some of the page sizes. I don't feel confident enough with this code to alter it. I'd expect to break the heuristics.
Best regards Thomas
-Daniel
Best regards Thomas
-Daniel
-- 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
On Thu, Aug 13, 2020 at 12:28:55PM +0200, Thomas Zimmermann wrote:
Am 13.08.20 um 11:48 schrieb Daniel Vetter:
On Thu, Aug 13, 2020 at 11:19:31AM +0200, Thomas Zimmermann wrote:
Hi
Am 07.08.20 um 18:10 schrieb Daniel Vetter:
On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 07.08.20 um 15:12 schrieb Daniel Vetter:
On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote: > The malidp driver uses GEM object functions for callbacks. Fix it to > use them internally as well. > > Signed-off-by: Thomas Zimmermann tzimmermann@suse.de > Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") > Cc: Thomas Zimmermann tzimmermann@suse.de > Cc: Emil Velikov emil.velikov@collabora.com > Cc: Liviu Dudau liviu.dudau@arm.com > Cc: Brian Starkey brian.starkey@arm.com > --- > drivers/gpu/drm/arm/malidp_planes.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c > index ab45ac445045..351a85088d0e 100644 > --- a/drivers/gpu/drm/arm/malidp_planes.c > +++ b/drivers/gpu/drm/arm/malidp_planes.c > @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, > if (cma_obj->sgt) > sgt = cma_obj->sgt; > else > - sgt = obj->dev->driver->gem_prime_get_sg_table(obj); > + sgt = obj->funcs->get_sg_table(obj);
Uh if there's not a switch somewhere I'd just call the right function directly. Or call the right wrapper for this, this feels a bit fishy ...
The driver initializes the pointer via CMA helper macro to an CMA-internal default. Calling the actual function here is fragile if the CMA-internal default ever changes.
But I have no strong feelings. I'll go with whatever the driver's maintainer prefers.
What I meant is: There should be an exported helper to get at the sgt. Drivers using helpers shouldn't need to do this kind of stuff here.
Also the entire code is fairly suspect, getting at the sgt from plane_check is a bit iffy. But that's a different kind of problem.
I tried to somehow move the code to CMA, but it's not easy. There's no good place to put the look-up code of sgt. And sgt is later being freed iff it came from the callback (and not freed if it was stored in the object). AFIACT the best options are to either keep the code here or move the entire function to CMA helpers.
Ok I read some code ... I'm confused. From the control flow it looks like malidp is using cma helpers. Otherwise why does the upcasting not blow up sometimes.
But cma helpers already check at import time that any imported dma-buf is contiguous, and they guarantee to fill out the cma_obj->sgt.
So really no idea what this code is doing here.
It's also not correct, since it doesn't coalesce sgt entries, since a range might be split up, but still mapped into a contiguous dma_addr_t range when you take it all together. The code in drm_gem_cma_prime_import_sg_table() gets this more right.
So maybe right fix is to just ditch this all, and use cma helpers fully?
The driver already does use CMA, including drm_gem_cma_prime_import_sg_table().
The patched code is not about importing/exporting sg tables. It configures the MMU's prefetching pattern by looking at some of the page sizes. I don't feel confident enough with this code to alter it. I'd expect to break the heuristics.
Hm ok, no idea either.
But then we can just assume that cma_obj->sgt is always set, and we don't have to call anything. If a driver uses cma helpers, and cma doesn't set ->sgt over the lifetime of the buffer, that breaks a cma helper assumption since cma doens't support swap-out.
So converting the if to a WARN_ON and failing with an error, and then remove the else should work. -Daniel
Best regards Thomas
-Daniel
Best regards Thomas
-Daniel
-- 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
-- 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
Hi
Am 13.08.20 um 12:31 schrieb Daniel Vetter:
On Thu, Aug 13, 2020 at 12:28:55PM +0200, Thomas Zimmermann wrote:
Am 13.08.20 um 11:48 schrieb Daniel Vetter:
On Thu, Aug 13, 2020 at 11:19:31AM +0200, Thomas Zimmermann wrote:
Hi
Am 07.08.20 um 18:10 schrieb Daniel Vetter:
On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 07.08.20 um 15:12 schrieb Daniel Vetter: > On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote: >> The malidp driver uses GEM object functions for callbacks. Fix it to >> use them internally as well. >> >> Signed-off-by: Thomas Zimmermann tzimmermann@suse.de >> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") >> Cc: Thomas Zimmermann tzimmermann@suse.de >> Cc: Emil Velikov emil.velikov@collabora.com >> Cc: Liviu Dudau liviu.dudau@arm.com >> Cc: Brian Starkey brian.starkey@arm.com >> --- >> drivers/gpu/drm/arm/malidp_planes.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c >> index ab45ac445045..351a85088d0e 100644 >> --- a/drivers/gpu/drm/arm/malidp_planes.c >> +++ b/drivers/gpu/drm/arm/malidp_planes.c >> @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, >> if (cma_obj->sgt) >> sgt = cma_obj->sgt; >> else >> - sgt = obj->dev->driver->gem_prime_get_sg_table(obj); >> + sgt = obj->funcs->get_sg_table(obj); > > Uh if there's not a switch somewhere I'd just call the right function > directly. Or call the right wrapper for this, this feels a bit fishy ...
The driver initializes the pointer via CMA helper macro to an CMA-internal default. Calling the actual function here is fragile if the CMA-internal default ever changes.
But I have no strong feelings. I'll go with whatever the driver's maintainer prefers.
What I meant is: There should be an exported helper to get at the sgt. Drivers using helpers shouldn't need to do this kind of stuff here.
Also the entire code is fairly suspect, getting at the sgt from plane_check is a bit iffy. But that's a different kind of problem.
I tried to somehow move the code to CMA, but it's not easy. There's no good place to put the look-up code of sgt. And sgt is later being freed iff it came from the callback (and not freed if it was stored in the object). AFIACT the best options are to either keep the code here or move the entire function to CMA helpers.
Ok I read some code ... I'm confused. From the control flow it looks like malidp is using cma helpers. Otherwise why does the upcasting not blow up sometimes.
But cma helpers already check at import time that any imported dma-buf is contiguous, and they guarantee to fill out the cma_obj->sgt.
So really no idea what this code is doing here.
It's also not correct, since it doesn't coalesce sgt entries, since a range might be split up, but still mapped into a contiguous dma_addr_t range when you take it all together. The code in drm_gem_cma_prime_import_sg_table() gets this more right.
So maybe right fix is to just ditch this all, and use cma helpers fully?
The driver already does use CMA, including drm_gem_cma_prime_import_sg_table().
The patched code is not about importing/exporting sg tables. It configures the MMU's prefetching pattern by looking at some of the page sizes. I don't feel confident enough with this code to alter it. I'd expect to break the heuristics.
Hm ok, no idea either.
But then we can just assume that cma_obj->sgt is always set, and we don't have to call anything. If a driver uses cma helpers, and cma doesn't set ->sgt over the lifetime of the buffer, that breaks a cma helper assumption since cma doens't support swap-out.
Really? I just looked at drm_gem_cma_helper.c and ->sgt is only ever set on imports, and only freed for imported memory. I'm confused now...
Best regards Thomas
So converting the if to a WARN_ON and failing with an error, and then remove the else should work. -Daniel
Best regards Thomas
-Daniel
Best regards Thomas
-Daniel
-- 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
-- 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
On Thu, Aug 13, 2020 at 12:39 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 13.08.20 um 12:31 schrieb Daniel Vetter:
On Thu, Aug 13, 2020 at 12:28:55PM +0200, Thomas Zimmermann wrote:
Am 13.08.20 um 11:48 schrieb Daniel Vetter:
On Thu, Aug 13, 2020 at 11:19:31AM +0200, Thomas Zimmermann wrote:
Hi
Am 07.08.20 um 18:10 schrieb Daniel Vetter:
On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann tzimmermann@suse.de wrote: > > Hi > > Am 07.08.20 um 15:12 schrieb Daniel Vetter: >> On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote: >>> The malidp driver uses GEM object functions for callbacks. Fix it to >>> use them internally as well. >>> >>> Signed-off-by: Thomas Zimmermann tzimmermann@suse.de >>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") >>> Cc: Thomas Zimmermann tzimmermann@suse.de >>> Cc: Emil Velikov emil.velikov@collabora.com >>> Cc: Liviu Dudau liviu.dudau@arm.com >>> Cc: Brian Starkey brian.starkey@arm.com >>> --- >>> drivers/gpu/drm/arm/malidp_planes.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c >>> index ab45ac445045..351a85088d0e 100644 >>> --- a/drivers/gpu/drm/arm/malidp_planes.c >>> +++ b/drivers/gpu/drm/arm/malidp_planes.c >>> @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, >>> if (cma_obj->sgt) >>> sgt = cma_obj->sgt; >>> else >>> - sgt = obj->dev->driver->gem_prime_get_sg_table(obj); >>> + sgt = obj->funcs->get_sg_table(obj); >> >> Uh if there's not a switch somewhere I'd just call the right function >> directly. Or call the right wrapper for this, this feels a bit fishy ... > > The driver initializes the pointer via CMA helper macro to an > CMA-internal default. Calling the actual function here is fragile if the > CMA-internal default ever changes. > > But I have no strong feelings. I'll go with whatever the driver's > maintainer prefers.
What I meant is: There should be an exported helper to get at the sgt. Drivers using helpers shouldn't need to do this kind of stuff here.
Also the entire code is fairly suspect, getting at the sgt from plane_check is a bit iffy. But that's a different kind of problem.
I tried to somehow move the code to CMA, but it's not easy. There's no good place to put the look-up code of sgt. And sgt is later being freed iff it came from the callback (and not freed if it was stored in the object). AFIACT the best options are to either keep the code here or move the entire function to CMA helpers.
Ok I read some code ... I'm confused. From the control flow it looks like malidp is using cma helpers. Otherwise why does the upcasting not blow up sometimes.
But cma helpers already check at import time that any imported dma-buf is contiguous, and they guarantee to fill out the cma_obj->sgt.
So really no idea what this code is doing here.
It's also not correct, since it doesn't coalesce sgt entries, since a range might be split up, but still mapped into a contiguous dma_addr_t range when you take it all together. The code in drm_gem_cma_prime_import_sg_table() gets this more right.
So maybe right fix is to just ditch this all, and use cma helpers fully?
The driver already does use CMA, including drm_gem_cma_prime_import_sg_table().
The patched code is not about importing/exporting sg tables. It configures the MMU's prefetching pattern by looking at some of the page sizes. I don't feel confident enough with this code to alter it. I'd expect to break the heuristics.
Hm ok, no idea either.
But then we can just assume that cma_obj->sgt is always set, and we don't have to call anything. If a driver uses cma helpers, and cma doesn't set ->sgt over the lifetime of the buffer, that breaks a cma helper assumption since cma doens't support swap-out.
Really? I just looked at drm_gem_cma_helper.c and ->sgt is only ever set on imports, and only freed for imported memory. I'm confused now...
Hm right this works differently than I thought, for native cma objects we just store padd/vaddr and that's it. Still feels wrong that malidp digs around in an internal helper that's meant for exporting as a dma-buf.
Oh well I guess this is just a very special special case.
Reviewed-by: Daniel Vetter daniel@ffwll.ch
Best regards Thomas
So converting the if to a WARN_ON and failing with an error, and then remove the else should work. -Daniel
Best regards Thomas
-Daniel
Best regards Thomas
-Daniel
-- 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
-- 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
-- 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
On Thu, Aug 13, 2020 at 02:01:19PM +0200, Daniel Vetter wrote:
On Thu, Aug 13, 2020 at 12:39 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 13.08.20 um 12:31 schrieb Daniel Vetter:
On Thu, Aug 13, 2020 at 12:28:55PM +0200, Thomas Zimmermann wrote:
Am 13.08.20 um 11:48 schrieb Daniel Vetter:
On Thu, Aug 13, 2020 at 11:19:31AM +0200, Thomas Zimmermann wrote:
Hi
Am 07.08.20 um 18:10 schrieb Daniel Vetter: > On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann tzimmermann@suse.de wrote: >> >> Hi >> >> Am 07.08.20 um 15:12 schrieb Daniel Vetter: >>> On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote: >>>> The malidp driver uses GEM object functions for callbacks. Fix it to >>>> use them internally as well. >>>> >>>> Signed-off-by: Thomas Zimmermann tzimmermann@suse.de >>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") >>>> Cc: Thomas Zimmermann tzimmermann@suse.de >>>> Cc: Emil Velikov emil.velikov@collabora.com >>>> Cc: Liviu Dudau liviu.dudau@arm.com >>>> Cc: Brian Starkey brian.starkey@arm.com >>>> --- >>>> drivers/gpu/drm/arm/malidp_planes.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c >>>> index ab45ac445045..351a85088d0e 100644 >>>> --- a/drivers/gpu/drm/arm/malidp_planes.c >>>> +++ b/drivers/gpu/drm/arm/malidp_planes.c >>>> @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, >>>> if (cma_obj->sgt) >>>> sgt = cma_obj->sgt; >>>> else >>>> - sgt = obj->dev->driver->gem_prime_get_sg_table(obj); >>>> + sgt = obj->funcs->get_sg_table(obj); >>> >>> Uh if there's not a switch somewhere I'd just call the right function >>> directly. Or call the right wrapper for this, this feels a bit fishy ... >> >> The driver initializes the pointer via CMA helper macro to an >> CMA-internal default. Calling the actual function here is fragile if the >> CMA-internal default ever changes. >> >> But I have no strong feelings. I'll go with whatever the driver's >> maintainer prefers.
Hi,
Sorry for the silence, I was on holiday.
> > What I meant is: There should be an exported helper to get at the sgt. > Drivers using helpers shouldn't need to do this kind of stuff here. > > Also the entire code is fairly suspect, getting at the sgt from > plane_check is a bit iffy. But that's a different kind of problem.
I tried to somehow move the code to CMA, but it's not easy. There's no good place to put the look-up code of sgt. And sgt is later being freed iff it came from the callback (and not freed if it was stored in the object). AFIACT the best options are to either keep the code here or move the entire function to CMA helpers.
Ok I read some code ... I'm confused. From the control flow it looks like malidp is using cma helpers. Otherwise why does the upcasting not blow up sometimes.
But cma helpers already check at import time that any imported dma-buf is contiguous, and they guarantee to fill out the cma_obj->sgt.
So really no idea what this code is doing here.
It's also not correct, since it doesn't coalesce sgt entries, since a range might be split up, but still mapped into a contiguous dma_addr_t range when you take it all together. The code in drm_gem_cma_prime_import_sg_table() gets this more right.
So maybe right fix is to just ditch this all, and use cma helpers fully?
The driver already does use CMA, including drm_gem_cma_prime_import_sg_table().
The patched code is not about importing/exporting sg tables. It configures the MMU's prefetching pattern by looking at some of the page sizes. I don't feel confident enough with this code to alter it. I'd expect to break the heuristics.
That's right, this piece of code has nothing to do with importing dma-buf or checking if things are contiguous. Mali DP hardware has a prefetcher block that can generate some fake requests during vblank to get the MMU pagetables cached so that reads from the buffers can be done without waiting for page walks. The block supports two modes, a full page or partial page request (to cater for the ends of the buffer that might not be a full page).
The patch proposed looks good to me, so:
Acked-by: Liviu Dudau liviu.dudau@arm.com
I will push the patch into drm-misc-next if Daniel has no other comments.
Best regards, Liviu
Hm ok, no idea either.
But then we can just assume that cma_obj->sgt is always set, and we don't have to call anything. If a driver uses cma helpers, and cma doesn't set ->sgt over the lifetime of the buffer, that breaks a cma helper assumption since cma doens't support swap-out.
Really? I just looked at drm_gem_cma_helper.c and ->sgt is only ever set on imports, and only freed for imported memory. I'm confused now...
Hm right this works differently than I thought, for native cma objects we just store padd/vaddr and that's it. Still feels wrong that malidp digs around in an internal helper that's meant for exporting as a dma-buf.
Oh well I guess this is just a very special special case.
Reviewed-by: Daniel Vetter daniel@ffwll.ch
Best regards Thomas
So converting the if to a WARN_ON and failing with an error, and then remove the else should work. -Daniel
Best regards Thomas
-Daniel
Best regards Thomas
> -Daniel >
-- 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
-- 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
-- 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
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Hi
Am 18.08.20 um 19:15 schrieb Liviu Dudau:
On Thu, Aug 13, 2020 at 02:01:19PM +0200, Daniel Vetter wrote:
On Thu, Aug 13, 2020 at 12:39 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 13.08.20 um 12:31 schrieb Daniel Vetter:
On Thu, Aug 13, 2020 at 12:28:55PM +0200, Thomas Zimmermann wrote:
Am 13.08.20 um 11:48 schrieb Daniel Vetter:
On Thu, Aug 13, 2020 at 11:19:31AM +0200, Thomas Zimmermann wrote: > Hi > > Am 07.08.20 um 18:10 schrieb Daniel Vetter: >> On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann tzimmermann@suse.de wrote: >>> >>> Hi >>> >>> Am 07.08.20 um 15:12 schrieb Daniel Vetter: >>>> On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote: >>>>> The malidp driver uses GEM object functions for callbacks. Fix it to >>>>> use them internally as well. >>>>> >>>>> Signed-off-by: Thomas Zimmermann tzimmermann@suse.de >>>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions") >>>>> Cc: Thomas Zimmermann tzimmermann@suse.de >>>>> Cc: Emil Velikov emil.velikov@collabora.com >>>>> Cc: Liviu Dudau liviu.dudau@arm.com >>>>> Cc: Brian Starkey brian.starkey@arm.com >>>>> --- >>>>> drivers/gpu/drm/arm/malidp_planes.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c >>>>> index ab45ac445045..351a85088d0e 100644 >>>>> --- a/drivers/gpu/drm/arm/malidp_planes.c >>>>> +++ b/drivers/gpu/drm/arm/malidp_planes.c >>>>> @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms, >>>>> if (cma_obj->sgt) >>>>> sgt = cma_obj->sgt; >>>>> else >>>>> - sgt = obj->dev->driver->gem_prime_get_sg_table(obj); >>>>> + sgt = obj->funcs->get_sg_table(obj); >>>> >>>> Uh if there's not a switch somewhere I'd just call the right function >>>> directly. Or call the right wrapper for this, this feels a bit fishy ... >>> >>> The driver initializes the pointer via CMA helper macro to an >>> CMA-internal default. Calling the actual function here is fragile if the >>> CMA-internal default ever changes. >>> >>> But I have no strong feelings. I'll go with whatever the driver's >>> maintainer prefers.
Hi,
Sorry for the silence, I was on holiday.
>> >> What I meant is: There should be an exported helper to get at the sgt. >> Drivers using helpers shouldn't need to do this kind of stuff here. >> >> Also the entire code is fairly suspect, getting at the sgt from >> plane_check is a bit iffy. But that's a different kind of problem. > > I tried to somehow move the code to CMA, but it's not easy. There's no > good place to put the look-up code of sgt. And sgt is later being freed > iff it came from the callback (and not freed if it was stored in the > object). AFIACT the best options are to either keep the code here or > move the entire function to CMA helpers.
Ok I read some code ... I'm confused. From the control flow it looks like malidp is using cma helpers. Otherwise why does the upcasting not blow up sometimes.
But cma helpers already check at import time that any imported dma-buf is contiguous, and they guarantee to fill out the cma_obj->sgt.
So really no idea what this code is doing here.
It's also not correct, since it doesn't coalesce sgt entries, since a range might be split up, but still mapped into a contiguous dma_addr_t range when you take it all together. The code in drm_gem_cma_prime_import_sg_table() gets this more right.
So maybe right fix is to just ditch this all, and use cma helpers fully?
The driver already does use CMA, including drm_gem_cma_prime_import_sg_table().
The patched code is not about importing/exporting sg tables. It configures the MMU's prefetching pattern by looking at some of the page sizes. I don't feel confident enough with this code to alter it. I'd expect to break the heuristics.
That's right, this piece of code has nothing to do with importing dma-buf or checking if things are contiguous. Mali DP hardware has a prefetcher block that can generate some fake requests during vblank to get the MMU pagetables cached so that reads from the buffers can be done without waiting for page walks. The block supports two modes, a full page or partial page request (to cater for the ends of the buffer that might not be a full page).
The patch proposed looks good to me, so:
Acked-by: Liviu Dudau liviu.dudau@arm.com
I will push the patch into drm-misc-next if Daniel has no other comments.
Thanks for explaining. The patch has already been merged.
Best regards Thomas
Best regards, Liviu
Hm ok, no idea either.
But then we can just assume that cma_obj->sgt is always set, and we don't have to call anything. If a driver uses cma helpers, and cma doesn't set ->sgt over the lifetime of the buffer, that breaks a cma helper assumption since cma doens't support swap-out.
Really? I just looked at drm_gem_cma_helper.c and ->sgt is only ever set on imports, and only freed for imported memory. I'm confused now...
Hm right this works differently than I thought, for native cma objects we just store padd/vaddr and that's it. Still feels wrong that malidp digs around in an internal helper that's meant for exporting as a dma-buf.
Oh well I guess this is just a very special special case.
Reviewed-by: Daniel Vetter daniel@ffwll.ch
Best regards Thomas
So converting the if to a WARN_ON and failing with an error, and then remove the else should work. -Daniel
Best regards Thomas
-Daniel
> > Best regards > Thomas > >> -Daniel >> > > -- > 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 >
-- 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
-- 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
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Liviu Dudau -
Thomas Zimmermann