Hi Liviu,
On 2016-06-08 11:05, liviu.dudau at arm.com wrote: > On Wed, Jun 08, 2016 at 08:58:33AM +0200, Marek Szyprowski wrote: >> On 2016-06-07 16:34, liviu.dudau at 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 at 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 at vger.kernel.org/msg97645.html Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland