Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752495AbbF3VFb (ORCPT ); Tue, 30 Jun 2015 17:05:31 -0400 Received: from forward2l.mail.yandex.net ([84.201.143.145]:33607 "EHLO forward2l.mail.yandex.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751780AbbF3VFX (ORCPT ); Tue, 30 Jun 2015 17:05:23 -0400 To: Laura Abbott Subject: Re: [PATCH v5.1 1/2] staging: ion: Add generic ion-physmem driver X-PHP-Originating-Script: 501:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Wed, 01 Jul 2015 00:05:15 +0300 From: Andrew Cc: Greg Kroah-Hartman , pebolle@tiscali.nl, Sudip Mukherjee , =?UTF-8?Q?Arve_Hj?= =?UTF-8?Q?=EF=BF=BDnnev=EF=BF=BDg?= , Riley Andrews , Chen Gang , Fabian Frederick , Android Kernel Team , linux-kernel@vger.kernel.org, john.stultz@linaro.org, devel@linuxdriverproject.org In-Reply-To: <5592D84B.6070801@redhat.com> References: <1435678490-7515-1-git-send-email-andrew@ncrmnt.org> <1435678490-7515-2-git-send-email-andrew@ncrmnt.org> <5592D84B.6070801@redhat.com> Message-ID: <083eee3944274129d962a29f86797654@mail.ncrmnt.org> User-Agent: Roundcube Webmail/1.0.2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9135 Lines: 300 Thanks for the detailed review Laura Abbott писал 30.06.2015 20:56: > On 06/30/2015 08:34 AM, Andrew Andrianov wrote: if (!idev) >> + return -ENOMEM; >> + } > > Yeah this is a bit messy as your comments note. Since there can only be > one Ion > device in the system, it seems like it would make more sense to have a > top level > DT node and then have the heaps be subnodes to avoid this 'guess when > to create > the device' bit. > I'm afraid this is not a very good idea, if the heaps represent some _physical_ address ranges, e.g. a SRAM memory (read below for our weird use case). In this case, the way I understand DT philosophy, it should be a subnode of the respective axi/apb/whatever node where it's connected. Correct me if I'm wrong. >> + >> + ipdev = kzalloc(sizeof(*ipdev), GFP_KERNEL); >> + if (!ipdev) >> + return -ENOMEM; >> + >> + platform_set_drvdata(pdev, ipdev); >> + >> + /* Read out name first for the sake of sane error-reporting */ >> + ret = of_property_read_string(pdev->dev.of_node, "ion-heap-name", >> + &ion_heap_name); >> + if (ret != 0) >> + goto errfreeipdev; >> + >> + ret = of_property_read_u32(pdev->dev.of_node, "ion-heap-id", >> + &ion_heap_id); >> + if (ret != 0) >> + goto errfreeipdev; >> + >> + /* Check id to be sane first */ >> + if ((ion_heap_id < 0) || (ion_heap_id >= ION_NUM_HEAP_IDS)) { >> + dev_err(&pdev->dev, "bad heap id specified: %d\n", >> + ion_heap_id); >> + goto errfreeipdev; >> + } >> + >> + if ((1 << ion_heap_id) & claimed_heap_ids) { >> + dev_err(&pdev->dev, "heap id %d is already claimed\n", >> + ion_heap_id); >> + goto errfreeipdev; >> + } > > I thought the core Ion framework was already checking this but > apparently not. This is so fundamental it should really be moved into > the core framework and not at the client level. I tried to mess as little as possible (e.g. not mess at all) with the guts of ION, so ended up with an extra check. If you want, I can hack into the ion itself, and send the patch for the next respin. > >> + >> + ret = of_property_read_u32(pdev->dev.of_node, "ion-heap-type", >> + &ion_heap_type); >> + if (ret != 0) >> + goto errfreeipdev; >> + >> + ret = of_property_read_u32(pdev->dev.of_node, "ion-heap-align", >> + &ion_heap_align); >> + if (ret != 0) >> + goto errfreeipdev; >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory"); >> + /* Not always needed, throw no error if missing */ >> + if (res) { >> + /* Fill in some defaults */ >> + addr = res->start; >> + size = resource_size(res); >> + } >> + >> + switch (ion_heap_type) { >> + case ION_HEAP_TYPE_DMA: >> + if (res) { >> + ret = dma_declare_coherent_memory(&pdev->dev, >> + res->start, >> + res->start, >> + resource_size(res), >> + DMA_MEMORY_MAP | >> + DMA_MEMORY_EXCLUSIVE); >> + if (ret == 0) { >> + ret = -ENODEV; >> + goto errfreeipdev; >> + } >> + } > > The name is ION_HEAP_TYPE_DMA but the real point of the heap was to > be able to use CMA memory. Calling dma_declare_coherent_memory defeats > the point since we won't use CMA memory. The coherent memory pool is > closer > to a carveout anyway so I'd argue if you want the effects of a coherent > memory pool you should be using carveout memory anyway. In our weird use case we use ION to share buffers between NeuroMatrix DSP cores, video decoder, video output device and a userspace application that orchestrates the whole process. The system has several dedicated SRAM banks, that should represented as dedicated ION heaps. Those are differently wired in the system (e.g. ARM core can't even execute code from some of them) and to achieve maximum performance certain buffers should be only allocated from certain banks for the thing to work fast. (Yes, it's just as scary as it sounds) In other words - we need several coherent pools, and dma_declare_coherent looked like the only existing way to tell ION: "hey, we want a heap out of this very physical region!" In reality that's mostly our only use case, all the rest heap types look like mostly useful for testing rather than in production, as a smarter replacement of ion-dummy driver which I used as a reference while writing this one. > >> + /* >> + * If no memory region declared in dt we assume that >> + * the user is okay with plain dma_alloc_coherent. >> + */ >> + break; >> + case ION_HEAP_TYPE_CARVEOUT: >> + case ION_HEAP_TYPE_CHUNK: >> + if (size == 0) { >> + ret = -EIO; >> + goto errfreeipdev; >> + } >> + ipdev->freepage_ptr = alloc_pages_exact(size, GFP_KERNEL); >> + if (ipdev->freepage_ptr) { >> + addr = virt_to_phys(ipdev->freepage_ptr); >> + } else { >> + ret = -ENOMEM; >> + goto errfreeipdev; >> + } >> + break; >> + } >> + > > This won't work if the carveout region is larger than the buddy > allocator > allows. That's why ion_reserve used the memblock APIs, to avoid being > limited by buddy allocator sizes. I guess it's okay for testing purposes. Unless I'm missing by the end of the workday a simple way to do it properly. > >> + ipdev->data.id = ion_heap_id; >> + ipdev->data.type = ion_heap_type; >> + ipdev->data.name = ion_heap_name; >> + ipdev->data.align = ion_heap_align; >> + ipdev->data.base = addr; >> + ipdev->data.size = size; >> + >> + /* This one make dma_declare_coherent_memory actually work */ >> + ipdev->data.priv = &pdev->dev; >> + >> + ipdev->heap = ion_heap_create(&ipdev->data); >> + if (!ipdev->heap) >> + goto errfreeipdev; >> + >> + /* If it's needed - take care enable clocks */ >> + ipdev->clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(ipdev->clk)) >> + ipdev->clk = NULL; >> + else >> + clk_prepare_enable(ipdev->clk); >> + > > Probe deferal for the clocks here? Oops, missed that one. Since I couldn't test clock gating (sram clock's not gated on my hw), I just settled for the same way they do it in drivers/misc/sram.c (And they don't handle deferral at all there!) > >> + ion_device_add_heap(idev, ipdev->heap); >> + claimed_heap_ids |= (1 << ion_heap_id); >> + ipdev->heap_id = ion_heap_id; >> + >> + dev_dbg(&pdev->dev, "heap %s id %d type %d align 0x%x at phys 0x%lx >> len %lu KiB\n", >> + ion_heap_name, ion_heap_id, ion_heap_type, ion_heap_align, >> + (unsigned long int)addr, ((unsigned long int)(size / 1024))); >> + return 0; >> + >> +errfreeipdev: >> + kfree(ipdev); >> + dev_err(&pdev->dev, "Failed to register heap: %s\n", >> + ion_heap_name); >> + return -ENOMEM; >> +} >> + >> +static int ion_physmem_remove(struct platform_device *pdev) >> +{ >> + struct physmem_ion_dev *ipdev = platform_get_drvdata(pdev); >> + >> + ion_heap_destroy(ipdev->heap); >> + claimed_heap_ids &= ~(1 << ipdev->heap_id); >> + if (ipdev->need_free_coherent) >> + dma_release_declared_memory(&pdev->dev); >> + if (ipdev->freepage_ptr) >> + free_pages_exact(ipdev->freepage_ptr, ipdev->data.size); >> + kfree(ipdev->heap); >> + if (ipdev->clk) >> + clk_disable_unprepare(ipdev->clk); >> + kfree(ipdev); >> + >> + /* We only remove heaps and disable clocks here. >> + * There's no point in nuking the device itself, since: >> + * a). ION driver modules can't be unloaded (yet?) >> + * b). ion_device_destroy() looks like a stub and doesn't >> + * take care to free heaps/clients. >> + * c). I can't think of a scenario where it will be required >> + */ >> + >> + return 0; >> +} >> + >> +static struct platform_driver ion_physmem_driver = { >> + .probe = ion_physmem_probe, >> + .remove = ion_physmem_remove, >> + .driver = { >> + .name = "ion-physmem", >> + .of_match_table = of_match_ptr(of_match_table), >> + }, >> +}; >> + >> +static int __init ion_physmem_init(void) >> +{ >> + return platform_driver_register(&ion_physmem_driver); >> +} >> +device_initcall(ion_physmem_init); >> diff --git a/include/dt-bindings/ion,physmem.h >> b/include/dt-bindings/ion,physmem.h >> new file mode 100644 >> index 0000000..6b24362 >> --- /dev/null >> +++ b/include/dt-bindings/ion,physmem.h >> @@ -0,0 +1,16 @@ >> +/* >> + * This header provides constants for ION physmem driver. >> + * >> + */ >> + >> +#ifndef __DT_BINDINGS_ION_PHYSMEM_H >> +#define __DT_BINDINGS_ION_PHYSMEM_H >> + >> +#define ION_HEAP_TYPE_SYSTEM 0 >> +#define ION_HEAP_TYPE_SYSTEM_CONTIG 1 >> +#define ION_HEAP_TYPE_CARVEOUT 2 >> +#define ION_HEAP_TYPE_CHUNK 3 >> +#define ION_HEAP_TYPE_DMA 4 >> +#define ION_HEAP_TYPE_CUSTOM 5 >> + >> +#endif /* __DT_BINDINGS_ION_PHYSMEM_H */ >> > > These are the same as the heap types in ion.h. If they actually need to > be > the same, they should be sharing definitions. If they don't need to be > the same, > please changes the name to avoid name space collisions. Got it, I'll make ion.h #include then. > > Thanks, > Laura -- Regards, Andrew RC Module :: http://module.ru -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/