From: Daniel Vetter <daniel@ffwll.ch> To: "Christian König" <ckoenig.leichtzumerken@gmail.com> Cc: "Rob Clark" <robdclark@gmail.com>, dri-devel <dri-devel@lists.freedesktop.org>, "Rob Clark" <robdclark@chromium.org>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <freedreno@lists.freedesktop.org>, "David Airlie" <airlied@linux.ie>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <linux-arm-msm@vger.kernel.org>, "open list" <linux-kernel@vger.kernel.org>, "Christian König" <christian.koenig@amd.com>, "moderated list:DMA BUFFER SHARING FRAMEWORK" <linaro-mm-sig@lists.linaro.org>, "Sean Paul" <sean@poorly.run>, "open list:DMA BUFFER SHARING FRAMEWORK" <linux-media@vger.kernel.org> Subject: Re: [Linaro-mm-sig] [PATCH] drm/msm: Add fence->wait() op Date: Thu, 22 Jul 2021 12:47:02 +0200 [thread overview] Message-ID: <YPlMpoGbXYKUFwCq@phenom.ffwll.local> (raw) In-Reply-To: <9c1e797b-8860-d1f5-e6f2-e06380ec9012@gmail.com> On Thu, Jul 22, 2021 at 11:28:01AM +0200, Christian König wrote: > Am 22.07.21 um 11:08 schrieb Daniel Vetter: > > [SNIP] > > > As far as I know wake_up_state() tries to run the thread on the CPU it was > > > scheduled last, while wait_event_* makes the thread run on the CPU who > > > issues the wake by default. > > > > > > And yes I've also noticed this already and it was one of the reason why I > > > suggested to use a wait_queue instead of the hand wired dma_fence_wait > > > implementation. > > The first versions had used wait_queue, but iirc we had some issues with > > the callbacks and stuff and that was the reasons for hand-rolling. Or > > maybe it was the integration of the lockless fastpath for > > dma_fence_is_signalled(). > > > > > [SNIP] > > > Well it would have been nicer if we used the existing infrastructure instead > > > of re-inventing stuff for dma_fence, but that chance is long gone. > > > > > > And you don't need a dma_fence_context base class, but rather just a flag in > > > the dma_fence_ops if you want to change the behavior. > > If there's something broken we should just fix it, not force everyone to > > set a random flag. dma_fence work like special wait_queues, so if we > > differ then we should go back to that. > > Wait a second with that, this is not broken. It's just different behavior > and there are good arguments for both sides. Oh I know, but since dma_fence is meant to be a wait_queue with hw support, they really should work the same and have the same tuning. > If a wait is short you can have situations where you want to start the > thread on the original CPU. > This is because you can assume that the caches on that CPU are still hot > and heating up the caches on the local CPU would take longer than an inter > CPU interrupt. > > But if the wait is long it makes more sense to run the thread on the CPU > where you noticed the wake up event. > This is because you can assume that the caches are cold anyway and > starting the thread on the current CPU (most likely from an interrupt > handler) gives you the absolutely best latency. > In other words you usually return from the interrupt handler and just > directly switch to the now running thread. > > I'm not sure if all drivers want the same behavior. Rob here seems to prefer > number 2, but we have used 1 for dma_fence for a rather long time now and it > could be that some people start to complain when we switch unconditionally. I think the defaults are different because usually if you wake up a wait queue, there's a 1:1 relationship between waker and waiter. Otoh if you just wake a thread it's probably some kinda of service thread, so N:1 relationship between waker and waiter. And in that case moving the waiter is a really bad idea. I think dma_fence is generally much closer to 1:1 (with the most common one irq handler -> scheduler thread for that engine), so having the "same cpu" wake behaviour really sounds like the right thing to do. And not anything that is specifically an issue with how qualcom gpus work, and hence should be msm specific. If it turns out to be the wrong thing, well I guess we'll learn something. And then maybe we have a different version of dma_fence_wait. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: "Christian König" <ckoenig.leichtzumerken@gmail.com> Cc: "Rob Clark" <robdclark@chromium.org>, "David Airlie" <airlied@linux.ie>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <linux-arm-msm@vger.kernel.org>, "open list" <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, "moderated list:DMA BUFFER SHARING FRAMEWORK" <linaro-mm-sig@lists.linaro.org>, "Sean Paul" <sean@poorly.run>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <freedreno@lists.freedesktop.org>, "Christian König" <christian.koenig@amd.com>, "open list:DMA BUFFER SHARING FRAMEWORK" <linux-media@vger.kernel.org> Subject: Re: [Linaro-mm-sig] [PATCH] drm/msm: Add fence->wait() op Date: Thu, 22 Jul 2021 12:47:02 +0200 [thread overview] Message-ID: <YPlMpoGbXYKUFwCq@phenom.ffwll.local> (raw) In-Reply-To: <9c1e797b-8860-d1f5-e6f2-e06380ec9012@gmail.com> On Thu, Jul 22, 2021 at 11:28:01AM +0200, Christian König wrote: > Am 22.07.21 um 11:08 schrieb Daniel Vetter: > > [SNIP] > > > As far as I know wake_up_state() tries to run the thread on the CPU it was > > > scheduled last, while wait_event_* makes the thread run on the CPU who > > > issues the wake by default. > > > > > > And yes I've also noticed this already and it was one of the reason why I > > > suggested to use a wait_queue instead of the hand wired dma_fence_wait > > > implementation. > > The first versions had used wait_queue, but iirc we had some issues with > > the callbacks and stuff and that was the reasons for hand-rolling. Or > > maybe it was the integration of the lockless fastpath for > > dma_fence_is_signalled(). > > > > > [SNIP] > > > Well it would have been nicer if we used the existing infrastructure instead > > > of re-inventing stuff for dma_fence, but that chance is long gone. > > > > > > And you don't need a dma_fence_context base class, but rather just a flag in > > > the dma_fence_ops if you want to change the behavior. > > If there's something broken we should just fix it, not force everyone to > > set a random flag. dma_fence work like special wait_queues, so if we > > differ then we should go back to that. > > Wait a second with that, this is not broken. It's just different behavior > and there are good arguments for both sides. Oh I know, but since dma_fence is meant to be a wait_queue with hw support, they really should work the same and have the same tuning. > If a wait is short you can have situations where you want to start the > thread on the original CPU. > This is because you can assume that the caches on that CPU are still hot > and heating up the caches on the local CPU would take longer than an inter > CPU interrupt. > > But if the wait is long it makes more sense to run the thread on the CPU > where you noticed the wake up event. > This is because you can assume that the caches are cold anyway and > starting the thread on the current CPU (most likely from an interrupt > handler) gives you the absolutely best latency. > In other words you usually return from the interrupt handler and just > directly switch to the now running thread. > > I'm not sure if all drivers want the same behavior. Rob here seems to prefer > number 2, but we have used 1 for dma_fence for a rather long time now and it > could be that some people start to complain when we switch unconditionally. I think the defaults are different because usually if you wake up a wait queue, there's a 1:1 relationship between waker and waiter. Otoh if you just wake a thread it's probably some kinda of service thread, so N:1 relationship between waker and waiter. And in that case moving the waiter is a really bad idea. I think dma_fence is generally much closer to 1:1 (with the most common one irq handler -> scheduler thread for that engine), so having the "same cpu" wake behaviour really sounds like the right thing to do. And not anything that is specifically an issue with how qualcom gpus work, and hence should be msm specific. If it turns out to be the wrong thing, well I guess we'll learn something. And then maybe we have a different version of dma_fence_wait. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
next prev parent reply other threads:[~2021-07-22 10:47 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-20 15:07 [PATCH] drm/msm: Add fence->wait() op Rob Clark 2021-07-20 15:07 ` Rob Clark 2021-07-20 18:03 ` [Linaro-mm-sig] " Christian König 2021-07-20 18:03 ` Christian König 2021-07-20 18:30 ` Rob Clark 2021-07-20 18:30 ` Rob Clark 2021-07-20 20:55 ` Daniel Vetter 2021-07-20 20:55 ` Daniel Vetter 2021-07-20 22:36 ` Rob Clark 2021-07-20 22:36 ` Rob Clark 2021-07-21 7:59 ` Daniel Vetter 2021-07-21 7:59 ` Daniel Vetter 2021-07-21 16:34 ` Rob Clark 2021-07-21 16:34 ` Rob Clark 2021-07-21 19:03 ` Daniel Vetter 2021-07-21 19:03 ` Daniel Vetter 2021-07-22 8:42 ` Christian König 2021-07-22 9:08 ` Daniel Vetter 2021-07-22 9:08 ` Daniel Vetter 2021-07-22 9:28 ` Christian König 2021-07-22 10:47 ` Daniel Vetter [this message] 2021-07-22 10:47 ` Daniel Vetter 2021-07-22 11:33 ` Christian König 2021-07-22 15:46 ` Rob Clark 2021-07-22 15:46 ` Rob Clark 2021-07-22 15:40 ` Rob Clark 2021-07-22 15:40 ` Rob Clark
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YPlMpoGbXYKUFwCq@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=airlied@linux.ie \ --cc=christian.koenig@amd.com \ --cc=ckoenig.leichtzumerken@gmail.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=linaro-mm-sig@lists.linaro.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=robdclark@chromium.org \ --cc=robdclark@gmail.com \ --cc=sean@poorly.run \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.