From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752869AbdBMMBf (ORCPT ); Mon, 13 Feb 2017 07:01:35 -0500 Received: from mailout1.w1.samsung.com ([210.118.77.11]:10551 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751945AbdBMMBc (ORCPT ); Mon, 13 Feb 2017 07:01:32 -0500 X-AuditID: cbfec7f4-f79716d000006f65-ad-58a1a017a5c2 Subject: Re: [PATCH v8 3/3] dmaengine: pl330: Don't require irq-safe runtime PM To: Vinod Koul , Ulf Hansson Cc: "Rafael J. Wysocki" , linux-samsung-soc , dmaengine@vger.kernel.org, "linux-arm-kernel@lists.infradead.org" , "linux-pm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Krzysztof Kozlowski , Bartlomiej Zolnierkiewicz , Lars-Peter Clausen , Arnd Bergmann , Inki Dae From: Marek Szyprowski Message-id: <7b0b47b2-8025-31e2-8180-6b1df63aa45f@samsung.com> Date: Mon, 13 Feb 2017 13:01:25 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-version: 1.0 In-reply-to: <20170213020340.GH2843@localhost> Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02SfyyUcRzH+3qe5+5x6/J0lI9f2a70RxbZaE+XmdTWs35NfzRWbXrwhOXn HaI/ZMYpRnaSm5G7OSpldEyYRReupZBff8i0WCdGJayTH+XuYfPf6/v9vL/v996ffUlM0kE4 kzHxyZw8no2VCkR4c89K31FHjTb02FTfCXpd1SOkX6rrCfp5yTpBq74U4XR/f4OQ1hVXErR+ apSgh9rKBfRiQRei1f2vbegPvYMEbawLoWcKO/BAMbP6V4WYqvYZG0Zfe1/AjI+2C5hG3V1G N2cgmKaRXJwpbKpFzKL+QLDtVZF/JBcbk8rJvQNuiKIXyruJxNnTaevmLmEm6vDLQ7YkUL7w x5hL8LwfBibqBXlIREqoagR15ibrQEItIsheT9p+MDleg/H3NQjK+114nkZQ8DnGwvbUZRhb GxRY2IE6D8PmJ0KLKUaV4NBSUWQ1FVA+kDefZxWJqQAoXJ60sTBOecC7ikKrZh91HQr0jzBe sxfMxRO4hW0pL5h9OmDVY5QMTBs5BM/u0PhiHrOEAbUghH/jU5sB5ObBDfSdGF/gDLxf1iOe 7WHW2CTk2RWGivNxnh8gyMrx5FmNoG9ezPNJeGv8tJW1B1TNpRhvL4Z7SgkvYaBNY9iyOQUj 37RCfqEaDLK78/Ei5F62o07ZjgplOypoEFaLHLgURVwUp/D1UrBxipT4KK+IhDg92vxWvRvG pRZU1SMzIIpE0t3iTKUmVEKwqYr0OAMCEpM6iLUV2lCJOJJNv8PJE8LkKbGcwoBcSFzqKG7X DIdIqCg2mbvFcYmcfHtqQ9o6ZyKng83VgiLnRPtd00ttc2ul51zSZK9M6dM6ZcXKDxS+2pOB fTUE2aXKwpIf2x2fa/jOqN2C16ZMnWy7t2vvWDz7s/ViUqBnelmOLGJCVelw9qMoPClwRK88 nFFdGWTn9+yS26Ffv6+pryhvVra2Ot82u+suZE6ef+PkYXoo8k/IkuKKaNbnCCZXsP8BAFRX ZFIDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrEIsWRmVeSWpSXmKPExsVy+t/xa7pSCxZGGJxfJGPxd9IxdouNM9az Wqye+pfVYtL9CSwW589vYLdYMnk+q8Wmx9dYLS7vmsNm8bn3CKPFjPP7mCzOnL7EanF8bbjF y779LA68Hr9/TWL0WLznJZPHplWdbB53ru1h89i8pN5jyZtDrB5brrazePRtWcXo8XmTXABn lJtNRmpiSmqRQmpecn5KZl66rVJoiJuuhZJCXmJuqq1ShK5vSJCSQlliTimQZ2SABhycA9yD lfTtEtwyPs45ylrwyrni748j7A2M+027GDk5JARMJB7dWcYMYYtJXLi3nq2LkYtDSGAJo8TW Jf+ZQBJCAs8ZJeZPlQGxhQX8JSZ92M4KYosIeEtc+bGcHaJhAbPE80kTmUAcZoHpLBLvpy8B 62YTMJToetvFBmLzCthJ9H19BBZnEVCVODG3D2ySqECMxN7++0wQNYISPybfYwGxOQX0JF6t uAAWZxYwk/jy8jArhC0vsXnNW+YJjAKzkLTMQlI2C0nZAkbmVYwiqaXFuem5xUZ6xYm5xaV5 6XrJ+bmbGIHxu+3Yzy07GLveBR9iFOBgVOLhbWhbECHEmlhWXJl7iFGCg1lJhHfh3IURQrwp iZVVqUX58UWlOanFhxhNgZ6YyCwlmpwPTC15JfGGJobmloZGxhYW5kZGSuK8Uz9cCRcSSE8s Sc1OTS1ILYLpY+LglGpgtFoueULR4/T3SSmnz3BHrVFxTpw8e4qCt5b466+2T1ZvLvFtu/th 5v8KBaYHl+4wFin85g2cekl67YPbZ8LeNyRMuW+zxeGcJoO9dMdmruen74rz5uvM3u6/8mqn 3J5Z/PqmkSXB4fUFCdfKrjUab2vX99kWXWIQz/WSI+vAQ5+vy8LcLd4oKLEUZyQaajEXFScC AISJ4aT1AgAA X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170213120127eucas1p1ae04ff700fc862282b533d3a2231f87c X-Msg-Generator: CA X-Sender-IP: 182.198.249.180 X-Local-Sender: =?UTF-8?B?TWFyZWsgU3p5cHJvd3NraRtTUlBPTC1LZXJuZWwgKFRQKRs=?= =?UTF-8?B?7IK87ISx7KCE7J6QG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Global-Sender: =?UTF-8?B?TWFyZWsgU3p5cHJvd3NraRtTUlBPTC1LZXJuZWwgKFRQKRtT?= =?UTF-8?B?YW1zdW5nIEVsZWN0cm9uaWNzG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTI=?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20170209142309eucas1p2b1277d96139eafc0d1dcc14145600476 X-RootMTR: 20170209142309eucas1p2b1277d96139eafc0d1dcc14145600476 References: <1486650171-20598-1-git-send-email-m.szyprowski@samsung.com> <1486650171-20598-4-git-send-email-m.szyprowski@samsung.com> <20170210045004.GN19244@localhost> <20170213020340.GH2843@localhost> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Vinod, On 2017-02-13 03:03, Vinod Koul wrote: > On Fri, Feb 10, 2017 at 02:57:09PM +0100, Ulf Hansson wrote: >> On 10 February 2017 at 12:51, Marek Szyprowski wrote: >>> On 2017-02-10 05:50, Vinod Koul wrote: >>>> On Thu, Feb 09, 2017 at 03:22:51PM +0100, Marek Szyprowski wrote: >>>>> +static int pl330_set_slave(struct dma_chan *chan, struct device *slave) >>>>> +{ >>>>> + struct dma_pl330_chan *pch = to_pchan(chan); >>>>> + struct pl330_dmac *pl330 = pch->dmac; >>>>> + int i; >>>>> + >>>>> + mutex_lock(&pl330->rpm_lock); >>>>> + >>>>> + for (i = 0; i < pl330->num_peripherals; i++) { >>>>> + if (pl330->peripherals[i].chan.slave == slave && >>>>> + pl330->peripherals[i].slave_link) { >>>>> + pch->slave_link = >>>>> pl330->peripherals[i].slave_link; >>>>> + goto done; >>>>> + } >>>>> + } >>>>> + >>>>> + pch->slave_link = device_link_add(slave, pl330->ddma.dev, >>>>> + DL_FLAG_PM_RUNTIME | >>>>> DL_FLAG_RPM_ACTIVE); >>>> So you are going to add the link on channel allocation and tear down on >>>> the >>>> freeup. >>> >>> Right. Channel allocation is typically done once per driver operation and it >>> won't hurt system performance. >>> >>>> I am not sure I really like the idea here. >>> >>> Could you point what's wrong with it? >>> >>>> First, these thing shouldn't be handled in the drivers. These things >>>> should >>>> be set in core and each driver setting the links doesn't sound great to >>>> me. >>> >>> Which core? And what's wrong with the device links? They have been >>> introduced to >>> model relations between devices that are behind the usual parent/child/bus >>> topology. >> I think Vinod mean the dmaengine core. Which also would make perfect >> sense to me as it would benefit all dma drivers. > Right. > >> The only related PM thing, that shall be the decision of the driver, >> is whether it wants to enable runtime PM or not, during ->probe(). > We can do pm_runtime_enabled() to check and that and do when enabled.. Another subtle issue is that there can be only one link between devices, but it is common to request more than one channel per client device (for example "tx" and "rx"), but this can be handled by internal reference counting. >>>> Second, should the link be always there and we only mange the state? Here >>>> it >>>> seems that we have link being created and destroyed, so why not mark it >>>> ACTIVE and DORMANT instead... >>> >>> Link state is managed by device core and should not be touched by the >>> drivers. >>> It is related to both provider and consumer drivers states (probed/not >>> probed/etc). >>> >>> Second we would need to create those links first. The question is where to >>> create them then. >> Just to fill in, to me this is really also the key question. >> >> If we could set up the device link already at device initialization, >> it should also be possible to avoid getting -EPROBE_DEFER for dma >> client drivers when requesting their dma channels. > Well if we defer then driver will regiser with dmaengine after it is > probed, so a client will either get a channel or not. IOW we won't get > -EPROBE_DEFER. I don't get how this will work. IMHO the link should be created WHEN client driver requests the channel, because otherwise we will get links that might be not used at all (for example optional DMA usage, but the link will force DMA controller to active state even if client device doesn't want to use DMA at all). So if client requests it for the first time and the DMA engine has not been probed yet, there is no way to avoid -EPROBE_DEFER. >>>> Lastly, looking at th description of the issue here, am perceiving (maybe >>>> my >>>> understanding is not quite right here) that you have an IP block in SoC >>>> which has multiple things and share common stuff and doing right PM is a >>>> challenge for you, right? >>> >>> Nope. Doing right PM in my SoC is not that complex and I would say it is >>> rather >>> typical for any embedded stuff. It works fine (in terms of the power >>> consumption reduction) when all drivers simply properly manage their runtime >>> PM state, thus if device is not in use, the state is set to suspended and >>> finally, the power domain gets turned off. >>> >>> I've used device links for PM only because the current DMA engine API is >>> simply insufficient to implement it in the other way. >>> >>> I want to let a power domain, which contains a few devices, among those a >>> PL330 >>> device, to get turned off when there is no activity. Handling power domain >>> power >>> on / off requires non-atomic context, what is typical for runtime pm calls. >>> For >>> that I need to have non-irq-safe runtime pm implemented for all devices that >>> belongs to that domains. >> Again, allow me to fill in. This issue exists for all ARM SoC which >> has a dma controller residing in a PM domain. I think that is quite >> many. >> >> Currently the only solution I have seen for this problem, but which I >> really dislike. That is, each dma client driver requests/releases >> their dma channel from their respective ->runtime_suspend|resume() >> callbacks - then the dma driver can use the dma request/release hooks, >> to do pm_runtime_get|put() which then becomes non-irq-safe. > Yeah that is not the best way to do. But looking at it current one doesnt > seem best fit either. > > So on seeing the device_link_add() I was thinking that this is some SoC > dependent problem being solved whereas the problem statmement is non-atomic > channel prepare. > > As I said earlier, if we want to solve that problem a better idea is to > actually split the prepare as we discussed in [1] > > This way we can get a non atomic descriptor allocate/prepare and release. > Yes we need to redesign the APIs to solve this, but if you guys are up for > it, I think we can do it and avoid any further round abouts :) I also agree that the main problem here is lack of non-atomic call for preparing the channel. However I don't feel I'm a right person for rewriting all the existing DMA engine drivers and clients for the new API. :/ >>> The problem with PL330 driver is that it use irq-safe runtime pm, which like >>> it >>> was stated in the patch description doesn't bring much benefits. To switch >>> to >>> standard (non-irq-safe) runtime pm, the pm_runtime calls have to be done >>> from >>> a context which permits sleeping. The problem with DMA engine driver API is >>> that >>> most of its callbacks have to be IRQ-safe and frankly only >>> device_{alloc,release}_chan_resources() what more or less maps to >>> dma_request_chan()/dma_release_channel() and friends. There are DMA engine >>> drivers which do runtime PM calls there (tegra20-apb-dma, sirf-dma, cppi41, >>> rcar-dmac), but this is not really efficient. DMA engine clients usually >>> allocate >>> dma channel during their probe() and keep them for the whole driver life. In >>> turn >>> this very similar to calling pm_runtime_get() in the DMA engine driver >>> probe(). >>> The result of both approaches is that DMA engine device keeps its power >>> domain >>> enabled almost all the time. This problem is also mentioned in the DMA >>> engine >>> TODO list, you have pointed me yesterday. >>> >>> To avoid such situation that DMA engine driver blocks turning off the power >>> domain and avoid changing DMA engine client API I came up with the device >>> links >>> pm based approach. I don't want to duplicate the description here, the >>> details >>> were in the patch description, however if you have any particular question >>> about >>> the details, let me know and I will try to clarify it more. >> So besides solving the irq-safe issue for dma driver, using the >> device-links has additionally two advantages. I already mentioned the >> -EPROBE_DEFER issue above. >> >> The second thing, is the runtime/system PM relations we get for free >> by using the links. In other words, the dma driver/core don't need to >> care about dealing with pm_runtime_get|put() as that would be managed >> by the dma client driver. > Yeah sorry took me a while to figure that out :), If we do a different API > then dmaengine core can call pm_runtime_get|put() from non-atomic context. > > [1]: http://www.spinics.net/lists/dmaengine/msg11570.html > Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland