Hi Daniel, Liviu,
Having just inadvertently merged -next into my working branch, I find dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a framebuffer, drm_gem_cma_create() fails, unconditionally calls the now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery death ensues...
Regards, Robin.
On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
Hi Daniel, Liviu,
Hi Robin,
Having just inadvertently merged -next into my working branch, I find dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a framebuffer, drm_gem_cma_create() fails, unconditionally calls the now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery death ensues...
Thanks for reporting this. What other changes other than reducing the CMA allocation size do you have that I might need in order to reproduce this?
Best regards, Liviu
Regards, Robin.
Hi Liviu,
On 07/06/16 14:35, liviu.dudau@arm.com wrote:
On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
Having just inadvertently merged -next into my working branch, I find dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a framebuffer, drm_gem_cma_create() fails, unconditionally calls the now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery death ensues...
Thanks for reporting this. What other changes other than reducing the CMA allocation size do you have that I might need in order to reproduce this?
I've just confirmed a plain checkout of next-20160602, using arm64 defconfig + DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the job:
[ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 3.044970] [drm] No driver support for vblank timestamp query. [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size 7680000 [ 3.084081] Bad mode in Synchronous Abort handler detected, code 0x86000004 -- IABT (current EL) [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted 4.7.0-rc1-next-20160602 #686 [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) [ 3.106567] Workqueue: deferwq deferred_probe_work_func [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: ffff8009768e8000 [ 3.119198] PC is at 0x0 [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 ...and so on.
Today's -next, on the other hand, dodges the bullet entirely:
[ 2.903645] [drm] found ARM HDLCD version r0p0 [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 [ 2.913505] tda998x: probe of 0-0070 failed with error -22 [ 2.919141] [drm] found ARM HDLCD version r0p0 [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 [ 2.928991] tda998x: probe of 0-0071 failed with error -22
Oh well...
Robin.
On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
Hi Liviu,
On 07/06/16 14:35, liviu.dudau@arm.com wrote:
On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
Having just inadvertently merged -next into my working branch, I find dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a framebuffer, drm_gem_cma_create() fails, unconditionally calls the now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery death ensues...
Thanks for reporting this. What other changes other than reducing the CMA allocation size do you have that I might need in order to reproduce this?
I've just confirmed a plain checkout of next-20160602, using arm64 defconfig
- DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the
job:
[ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 3.044970] [drm] No driver support for vblank timestamp query. [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size 7680000 [ 3.084081] Bad mode in Synchronous Abort handler detected, code 0x86000004 -- IABT (current EL) [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted 4.7.0-rc1-next-20160602 #686 [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) [ 3.106567] Workqueue: deferwq deferred_probe_work_func [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: ffff8009768e8000 [ 3.119198] PC is at 0x0 [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 ...and so on.
Today's -next, on the other hand, dodges the bullet entirely:
[ 2.903645] [drm] found ARM HDLCD version r0p0 [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 [ 2.913505] tda998x: probe of 0-0070 failed with error -22 [ 2.919141] [drm] found ARM HDLCD version r0p0 [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 [ 2.928991] tda998x: probe of 0-0071 failed with error -22
Yes, if fails in of_reserved_mem_device_init(). Tracking now to see which of the conditions at line 311 are the culprits.
Best regards, Liviu
Oh well...
Robin.
On Tue, Jun 07, 2016 at 03:14:04PM +0100, liviu.dudau@arm.com wrote:
On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
Hi Liviu,
On 07/06/16 14:35, liviu.dudau@arm.com wrote:
On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
Having just inadvertently merged -next into my working branch, I find dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a framebuffer, drm_gem_cma_create() fails, unconditionally calls the now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery death ensues...
Thanks for reporting this. What other changes other than reducing the CMA allocation size do you have that I might need in order to reproduce this?
I've just confirmed a plain checkout of next-20160602, using arm64 defconfig
- DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the
job:
[ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 3.044970] [drm] No driver support for vblank timestamp query. [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size 7680000 [ 3.084081] Bad mode in Synchronous Abort handler detected, code 0x86000004 -- IABT (current EL) [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted 4.7.0-rc1-next-20160602 #686 [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) [ 3.106567] Workqueue: deferwq deferred_probe_work_func [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: ffff8009768e8000 [ 3.119198] PC is at 0x0 [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 ...and so on.
Today's -next, on the other hand, dodges the bullet entirely:
[ 2.903645] [drm] found ARM HDLCD version r0p0 [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 [ 2.913505] tda998x: probe of 0-0070 failed with error -22 [ 2.919141] [drm] found ARM HDLCD version r0p0 [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 [ 2.928991] tda998x: probe of 0-0071 failed with error -22
Yes, if fails in of_reserved_mem_device_init(). Tracking now to see which of the conditions at line 311 are the culprits.
Bah, discard the line number, old cached version in my editor.
Liviu
Best regards, Liviu
Oh well...
Robin.
On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
Hi Liviu,
On 07/06/16 14:35, liviu.dudau@arm.com wrote:
On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
Having just inadvertently merged -next into my working branch, I find dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a framebuffer, drm_gem_cma_create() fails, unconditionally calls the now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery death ensues...
Thanks for reporting this. What other changes other than reducing the CMA allocation size do you have that I might need in order to reproduce this?
I've just confirmed a plain checkout of next-20160602, using arm64 defconfig
- DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the
job:
[ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 3.044970] [drm] No driver support for vblank timestamp query. [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size 7680000 [ 3.084081] Bad mode in Synchronous Abort handler detected, code 0x86000004 -- IABT (current EL) [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted 4.7.0-rc1-next-20160602 #686 [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) [ 3.106567] Workqueue: deferwq deferred_probe_work_func [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: ffff8009768e8000 [ 3.119198] PC is at 0x0 [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 ...and so on.
Today's -next, on the other hand, dodges the bullet entirely:
[ 2.903645] [drm] found ARM HDLCD version r0p0 [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 [ 2.913505] tda998x: probe of 0-0070 failed with error -22 [ 2.919141] [drm] found ARM HDLCD version r0p0 [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 [ 2.928991] tda998x: probe of 0-0071 failed with error -22
OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from when there is no "memory-region" phandle in the DT: before it used to return -ENODEV, now it returns -EINVAL.
Marek, I quite liked the old behaviour to detect if the DT had the optional (from my driver's point of view) "memory-region" phandle. Plus the check for dev is superfluous when using of_reserved_mem_device_init() as that uses dev->of_node for np so it would crash before the check anyway. Maybe move the check there?
Until then I suggest reverting the 59ce4039727ef40 commit.
Best regards, Liviu
Oh well...
Robin.
Change return value back to -ENODEV when no region is defined for given device. This restores old behavior of this function, as some drivers rely on such error code.
Reported-by: Liviu Dudau liviu.dudau@arm.com Fixes: 59ce4039727ef40 ("of: reserved_mem: add support for using more than one region for given device") Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/of/of_reserved_mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 3cf129f..06af99f 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -334,7 +334,7 @@ int of_reserved_mem_device_init_by_idx(struct device *dev,
target = of_parse_phandle(np, "memory-region", idx); if (!target) - return -EINVAL; + return -ENODEV;
rmem = __find_rmem(target); of_node_put(target);
On 06/08/2016 08:51 AM, Marek Szyprowski wrote:
Change return value back to -ENODEV when no region is defined for given device. This restores old behavior of this function, as some drivers rely on such error code.
Reported-by: Liviu Dudau liviu.dudau@arm.com Fixes: 59ce4039727ef40 ("of: reserved_mem: add support for using more than one region for given device") Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
Reviewed-by: Sylwester Nawrocki s.nawrocki@samsung.com
I think this needs to be added to the media tree, where the original patch it fixes was applied.
On Wed, Jun 08, 2016 at 08:51:53AM +0200, Marek Szyprowski wrote:
Change return value back to -ENODEV when no region is defined for given device. This restores old behavior of this function, as some drivers rely on such error code.
Reported-by: Liviu Dudau liviu.dudau@arm.com Fixes: 59ce4039727ef40 ("of: reserved_mem: add support for using more than one region for given device") Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
Reviewed-by: Liviu Dudau Liviu.Dudau@arm.com
drivers/of/of_reserved_mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 3cf129f..06af99f 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -334,7 +334,7 @@ int of_reserved_mem_device_init_by_idx(struct device *dev,
target = of_parse_phandle(np, "memory-region", idx); if (!target)
return -EINVAL;
return -ENODEV;rmem = __find_rmem(target); of_node_put(target);
-- 1.9.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Jun 8, 2016 at 1:51 AM, Marek Szyprowski m.szyprowski@samsung.com wrote:
Change return value back to -ENODEV when no region is defined for given device. This restores old behavior of this function, as some drivers rely on such error code.
Reported-by: Liviu Dudau liviu.dudau@arm.com Fixes: 59ce4039727ef40 ("of: reserved_mem: add support for using more than one region for given device") Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/of/of_reserved_mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Rob Herring robh@kernel.org
On 8 June 2016 at 18:35, Rob Herring robh@kernel.org wrote:
On Wed, Jun 8, 2016 at 1:51 AM, Marek Szyprowski m.szyprowski@samsung.com wrote:
Change return value back to -ENODEV when no region is defined for given device. This restores old behavior of this function, as some drivers rely on such error code.
Reported-by: Liviu Dudau liviu.dudau@arm.com Fixes: 59ce4039727ef40 ("of: reserved_mem: add support for using more than one region for given device") Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/of/of_reserved_mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Looks reasonable; FWIW Reviewed-by: Sumit Semwal sumit.semwal@linaro.org
Acked-by: Rob Herring robh@kernel.org _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi All,
On 2016-06-07 16:34, liviu.dudau@arm.com wrote:
On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
Hi Liviu,
On 07/06/16 14:35, liviu.dudau@arm.com wrote:
On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
Having just inadvertently merged -next into my working branch, I find dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a framebuffer, drm_gem_cma_create() fails, unconditionally calls the now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery death ensues...
Thanks for reporting this. What other changes other than reducing the CMA allocation size do you have that I might need in order to reproduce this?
I've just confirmed a plain checkout of next-20160602, using arm64 defconfig
- DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the
job:
[ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 3.044970] [drm] No driver support for vblank timestamp query. [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size 7680000 [ 3.084081] Bad mode in Synchronous Abort handler detected, code 0x86000004 -- IABT (current EL) [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted 4.7.0-rc1-next-20160602 #686 [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) [ 3.106567] Workqueue: deferwq deferred_probe_work_func [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: ffff8009768e8000 [ 3.119198] PC is at 0x0 [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 ...and so on.
Today's -next, on the other hand, dodges the bullet entirely:
[ 2.903645] [drm] found ARM HDLCD version r0p0 [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 [ 2.913505] tda998x: probe of 0-0070 failed with error -22 [ 2.919141] [drm] found ARM HDLCD version r0p0 [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 [ 2.928991] tda998x: probe of 0-0071 failed with error -22
OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from when there is no "memory-region" phandle in the DT: before it used to return -ENODEV, now it returns -EINVAL.
Marek, I quite liked the old behaviour to detect if the DT had the optional (from my driver's point of view) "memory-region" phandle. Plus the check for dev is superfluous when using of_reserved_mem_device_init() as that uses dev->of_node for np so it would crash before the check anyway. Maybe move the check there?
Until then I suggest reverting the 59ce4039727ef40 commit.
I've just send a fix for this issue. I'm sorry for the regression. I hope the fix fill quickly get into next to solve your problem.
The additional check for null dev make sense, because the new function of_reserved_mem_device_init_by_idx can be called with any device and node pointer not embedded with it, so I would like to keep it safe.
Best regards
On Wed, Jun 08, 2016 at 08:58:33AM +0200, Marek Szyprowski wrote:
Hi All,
Hi Marek,
On 2016-06-07 16:34, liviu.dudau@arm.com wrote:
On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
Hi Liviu,
On 07/06/16 14:35, liviu.dudau@arm.com wrote:
On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
Having just inadvertently merged -next into my working branch, I find dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a framebuffer, drm_gem_cma_create() fails, unconditionally calls the now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery death ensues...
Thanks for reporting this. What other changes other than reducing the CMA allocation size do you have that I might need in order to reproduce this?
I've just confirmed a plain checkout of next-20160602, using arm64 defconfig
- DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the
job:
[ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 3.044970] [drm] No driver support for vblank timestamp query. [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size 7680000 [ 3.084081] Bad mode in Synchronous Abort handler detected, code 0x86000004 -- IABT (current EL) [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted 4.7.0-rc1-next-20160602 #686 [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) [ 3.106567] Workqueue: deferwq deferred_probe_work_func [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: ffff8009768e8000 [ 3.119198] PC is at 0x0 [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 ...and so on.
Today's -next, on the other hand, dodges the bullet entirely:
[ 2.903645] [drm] found ARM HDLCD version r0p0 [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 [ 2.913505] tda998x: probe of 0-0070 failed with error -22 [ 2.919141] [drm] found ARM HDLCD version r0p0 [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 [ 2.928991] tda998x: probe of 0-0071 failed with error -22
OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from when there is no "memory-region" phandle in the DT: before it used to return -ENODEV, now it returns -EINVAL.
Marek, I quite liked the old behaviour to detect if the DT had the optional (from my driver's point of view) "memory-region" phandle. Plus the check for dev is superfluous when using of_reserved_mem_device_init() as that uses dev->of_node for np so it would crash before the check anyway. Maybe move the check there?
Until then I suggest reverting the 59ce4039727ef40 commit.
I've just send a fix for this issue. I'm sorry for the regression. I hope the fix fill quickly get into next to solve your problem.
Thanks for the patch, however I have some comments to it.
The additional check for null dev make sense, because the new function of_reserved_mem_device_init_by_idx can be called with any device and node pointer not embedded with it, so I would like to keep it safe.
And I have to admit I find that scary. Why do you accept any node that is *not* related to the device? If you want just the ability to parse multiple "memory-region" phandles (where are the bindings defined for that?) I would have modified of_reserved_mem_device_init() to the the parsing and accept either the single entry style or a node with multiple "memory-region" phandles in it. Otherwise I can steal the "memory-region" of another device and that device would have no idea that I have done this.
Can you point me to the latest thread where this patch has been discussed?
Best regards, Liviu
Best regards
Marek Szyprowski, PhD Samsung R&D Institute Poland
Hi Liviu,
On 2016-06-08 11:05, liviu.dudau@arm.com wrote:
On Wed, Jun 08, 2016 at 08:58:33AM +0200, Marek Szyprowski wrote:
On 2016-06-07 16:34, liviu.dudau@arm.com wrote:
On Tue, Jun 07, 2016 at 03:11:14PM +0100, Robin Murphy wrote:
Hi Liviu,
On 07/06/16 14:35, liviu.dudau@arm.com wrote:
On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
Having just inadvertently merged -next into my working branch, I find dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a framebuffer, drm_gem_cma_create() fails, unconditionally calls the now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery death ensues...
Thanks for reporting this. What other changes other than reducing the CMA allocation size do you have that I might need in order to reproduce this?
I've just confirmed a plain checkout of next-20160602, using arm64 defconfig
- DRM + HDLCD + TDA998X and CMA_SIZE_MBYTES=1, booted on a Juno, does the
job:
[ 3.032402] hdlcd 7ff60000.hdlcd: bound 0-0070 (ops tda998x_ops) [ 3.038388] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 3.044970] [drm] No driver support for vblank timestamp query. [ 3.076973] hdlcd 7ff60000.hdlcd: failed to allocate buffer with size 7680000 [ 3.084081] Bad mode in Synchronous Abort handler detected, code 0x86000004 -- IABT (current EL) [ 3.092815] CPU: 3 PID: 6 Comm: kworker/u12:0 Not tainted 4.7.0-rc1-next-20160602 #686 [ 3.100682] Hardware name: ARM Juno development board (r1) (DT) [ 3.106567] Workqueue: deferwq deferred_probe_work_func [ 3.111761] task: ffff8009768a3e80 ti: ffff8009768e8000 task.ti: ffff8009768e8000 [ 3.119198] PC is at 0x0 [ 3.121720] LR is at drm_gem_cma_create+0x128/0x130 ...and so on.
Today's -next, on the other hand, dodges the bullet entirely:
[ 2.903645] [drm] found ARM HDLCD version r0p0 [ 2.908122] hdlcd 7ff60000.hdlcd: master bind failed: -22 [ 2.913505] tda998x: probe of 0-0070 failed with error -22 [ 2.919141] [drm] found ARM HDLCD version r0p0 [ 2.923609] hdlcd 7ff50000.hdlcd: master bind failed: -22 [ 2.928991] tda998x: probe of 0-0071 failed with error -22
OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from when there is no "memory-region" phandle in the DT: before it used to return -ENODEV, now it returns -EINVAL.
Marek, I quite liked the old behaviour to detect if the DT had the optional (from my driver's point of view) "memory-region" phandle. Plus the check for dev is superfluous when using of_reserved_mem_device_init() as that uses dev->of_node for np so it would crash before the check anyway. Maybe move the check there?
Until then I suggest reverting the 59ce4039727ef40 commit.
I've just send a fix for this issue. I'm sorry for the regression. I hope the fix fill quickly get into next to solve your problem.
Thanks for the patch, however I have some comments to it.
The additional check for null dev make sense, because the new function of_reserved_mem_device_init_by_idx can be called with any device and node pointer not embedded with it, so I would like to keep it safe.
And I have to admit I find that scary. Why do you accept any node that is *not* related to the device? If you want just the ability to parse multiple "memory-region" phandles (where are the bindings defined for that?) I would have modified of_reserved_mem_device_init() to the the parsing and accept either the single entry style or a node with multiple "memory-region" phandles in it. Otherwise I can steal the "memory-region" of another device and that device would have no idea that I have done this.
The idea is not to steal memory region from another device, but to let driver to use multiple regions assigned to the supported device with dma-mapping api. To use them with dma-mapping subsystem, one needs separate struct device for each reserved region. The idea is to create child devices of the device, which has memory-region property. Then for each such child device, driver can call of_reserved_mem_device_init_by_idx() to enable usage of dma-mapping api based on the selected reserved region. Such approach was already used for long time in the media/platform/s5p-mfc driver and now it has been converted to use generic reserved memory regions.
If you feel scary about this approach, maybe a check if the device provided to of_reserved_mem_device_init_by_idx() function is one of the successors of the device hidden behind the provided node pointer (or points to the same device)?
Can you point me to the latest thread where this patch has been discussed?
This patch is a part of "Exynos: MFC driver: reserved memory cleanup and IOMMU support" thread and has been around for a while: https://www.mail-archive.com/linux-media@vger.kernel.org/msg97645.html
Best regards
On Wed, Jun 08, 2016 at 12:01:20PM +0200, Marek Szyprowski wrote:
Hi Liviu,
Hi Marek,
[HDLCD bug report content removed]
OK, the problem is that commit 59ce4039727ef40 has changed the behaviour from when there is no "memory-region" phandle in the DT: before it used to return -ENODEV, now it returns -EINVAL.
Marek, I quite liked the old behaviour to detect if the DT had the optional (from my driver's point of view) "memory-region" phandle. Plus the check for dev is superfluous when using of_reserved_mem_device_init() as that uses dev->of_node for np so it would crash before the check anyway. Maybe move the check there?
Until then I suggest reverting the 59ce4039727ef40 commit.
I've just send a fix for this issue. I'm sorry for the regression. I hope the fix fill quickly get into next to solve your problem.
Thanks for the patch, however I have some comments to it.
The additional check for null dev make sense, because the new function of_reserved_mem_device_init_by_idx can be called with any device and node pointer not embedded with it, so I would like to keep it safe.
And I have to admit I find that scary. Why do you accept any node that is *not* related to the device? If you want just the ability to parse multiple "memory-region" phandles (where are the bindings defined for that?) I would have modified of_reserved_mem_device_init() to the the parsing and accept either the single entry style or a node with multiple "memory-region" phandles in it. Otherwise I can steal the "memory-region" of another device and that device would have no idea that I have done this.
The idea is not to steal memory region from another device, but to let driver to use multiple regions assigned to the supported device with dma-mapping api. To use them with dma-mapping subsystem, one needs separate struct device for each reserved region. The idea is to create child devices of the device, which has memory-region property. Then for each such child device, driver can call of_reserved_mem_device_init_by_idx() to enable usage of dma-mapping api based on the selected reserved region. Such approach was already used for long time in the media/platform/s5p-mfc driver and now it has been converted to use generic reserved memory regions.
If you feel scary about this approach, maybe a check if the device provided to of_reserved_mem_device_init_by_idx() function is one of the successors of the device hidden behind the provided node pointer (or points to the same device)?
That would not hurt to have.
Can you point me to the latest thread where this patch has been discussed?
This patch is a part of "Exynos: MFC driver: reserved memory cleanup and IOMMU support" thread and has been around for a while: https://www.mail-archive.com/linux-media@vger.kernel.org/msg97645.html
Thanks! I'm not subscribed to linux-media and way behind a lot of other MLs, so I have not noticed it.
I've had a look at the patchset, found one case where you might leak some memory.
I also think you could've had an of_reserved_mem_device_init_list() function that inits all the "memory-region" nodes and returns a list struct reserved_mem* that the driver can then assign to whatever internal variables it has. That way you can validate that all the "memory-region" phandles are used by the same parent device.
Best regards, Liviu
Best regards
Marek Szyprowski, PhD Samsung R&D Institute Poland
On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
Hi Daniel, Liviu,
Having just inadvertently merged -next into my working branch, I find dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a framebuffer, drm_gem_cma_create() fails, unconditionally calls the now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery death ensues...
Please make sure you have the drm fixes from 4.7-rc2 included, this issue is fixed there. -Daniel
On 07/06/16 15:40, Daniel Vetter wrote:
On Tue, Jun 07, 2016 at 01:06:00PM +0100, Robin Murphy wrote:
Hi Daniel, Liviu,
Having just inadvertently merged -next into my working branch, I find dev6d910bfa809e ("drm/hlcd: Use lockless gem BO free callback") adversely affecting my board's ability to boot ;)
Since I (intentionally) don't have sufficient CMA to create a framebuffer, drm_gem_cma_create() fails, unconditionally calls the now-NULL drm->driver->gem_free_object() in its cleanup path, and fiery death ensues...
Please make sure you have the drm fixes from 4.7-rc2 included, this issue is fixed there.
Ah, right you are, thanks - the fact that that the newer branches I tried crashed or otherwise failed due to unrelated TDA998x problems threw me off. I see Chris' fix there now.
Robin.
-Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Liviu Dudau -
liviu.dudau＠arm.com
-
Marek Szyprowski -
Rob Herring -
Robin Murphy -
Sumit Semwal -
Sylwester Nawrocki