Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Mon, Oct 29, 2018 at 10:10:56AM +0100, Boris Brezillon wrote: > On Mon, 29 Oct 2018 10:03:01 +0100 > Daniel Vetter wrote: > > On Mon, Oct 29, 2018 at 09:41:36AM +0100, Boris Brezillon wrote: > > > On Mon, 29 Oct 2018 09:06:40 +0100 > > > > The other upshot of a counter is that there's no problem with resetting. > > > > Userspace simply grabs the counter before and after the test and > > > > compares. > > > > If you only have 0/1 you need some way to reset, or otherwise automated > > > > running in a CI farm isn't possible. > > > > > > The reset was done during atomic commit in my proposal, so no action > > > was required on the user side apart from applying a new config. > > > Anyway, I'll change that for a real counter. > > > > We want to measure underruns while doing full modesets too (specifically, > > when shutting down the pipeline). At least for our hw this has > > historically helped greatly in catching programming sequence issues. > > Looks like you have particular needs for intel HW which I don't know > about, maybe I shouldn't be the one driving this rework... No one is expecting you do drive i915 work, we just want to make sure that whatever you come up for igt tests is generally useful, and not just only for the limited use case of vc4. Atm the only two drivers interested in underrun reporting are i915 and vc4, and it looks like they span a pretty broad spectrum alredy. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Mon, 29 Oct 2018 10:03:01 +0100 Daniel Vetter wrote: > On Mon, Oct 29, 2018 at 09:41:36AM +0100, Boris Brezillon wrote: > > On Mon, 29 Oct 2018 09:06:40 +0100 > > Daniel Vetter wrote: > > > > > On Fri, Oct 26, 2018 at 06:10:03PM +0300, Ville Syrjälä wrote: > > > > On Fri, Oct 26, 2018 at 04:52:33PM +0200, Boris Brezillon wrote: > > > > > On Fri, 26 Oct 2018 16:26:03 +0200 > > > > > Daniel Vetter wrote: > > > > > > > > > > > On Fri, Oct 26, 2018 at 3:57 PM Boris Brezillon > > > > > > wrote: > > > > > > > > > > > > > > On Fri, 26 Oct 2018 15:30:26 +0200 > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > On Thu, Oct 25, 2018 at 11:41:14AM +0200, Boris Brezillon > > > > > > > > wrote: > > > > > > > > > On Thu, 25 Oct 2018 11:33:14 +0200 > > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > > On Thu, Oct 25, 2018 at 10:09:31AM +0200, Boris Brezillon > > > > > > > > > > wrote: > > > > > > > > > > > On Tue, 23 Oct 2018 15:44:43 +0200 > > > > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > > > > > > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris > > > > > > > > > > > > Brezillon wrote: > > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > > > > > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris > > > > > > > > > > > > > > Brezillon wrote: > > > > > > > > > > > > > > > The HVS block is supposed to fill the pixelvalve > > > > > > > > > > > > > > > FIFOs fast enough to > > > > > > > > > > > > > > > meet the requested framerate. The problem is, the > > > > > > > > > > > > > > > HVS and memory bus > > > > > > > > > > > > > > > bandwidths are limited, and if we don't take > > > > > > > > > > > > > > > these limitations into > > > > > > > > > > > > > > > account we might end up with HVS underflow errors. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This patch is trying to model the per-plane HVS > > > > > > > > > > > > > > > and memory bus bandwidth > > > > > > > > > > > > > > > consumption and take a decision at atomic_check() > > > > > > > > > > > > > > > time whether the > > > > > > > > > > > > > > > estimated load will fit in the HVS and membus > > > > > > > > > > > > > > > budget. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Note that we take an extra margin on the memory > > > > > > > > > > > > > > > bus consumption to let > > > > > > > > > > > > > > > the system run smoothly when other blocks are > > > > > > > > > > > > > > > doing heavy use of the > > > > > > > > > > > > > > > memory bus. Same goes for the HVS limit, except > > > > > > > > > > > > > > > the margin is smaller in > > > > > > > > > > > > > > > this case, since the HVS is not used by external > > > > > > > > > > > > > > > components. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > This logic has been validated using a simple > > > > > > > > > > > > > > > shell script and > > > > > > > > > > > > > > > some instrumentation in the VC4 driver: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > - capture underflow errors at the HVS level and > > > > > > > > > > > > > > > expose a debugfs file > > > > > > > > > > > > > > > reporting those errors > > > > > > > > > > > > > > > - add debugfs files to expose when atomic_check > > > > > > > > > > > > > > > fails because of the > > > > > > > > > > > > > > > HVS or membus load limitation or when it fails > > > > > > > > > > > > > > > for other reasons > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The branch containing those modification is > > > > > > > > > > > > > > > available here [1], and the > > > > > > > > > > > > > > > script (which is internally using modetest) is > > > > > > > > > > > > > > > here [2] (please note > > > > > > > > > > > > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Note that those modification tend to > > > > > > > > > > > > > > > over-estimate the load, and thus > > > > > > > > > > > > > > > reject setups that might have previously worked, > > > > > > > > > > > > > > > so we might want to > > > > > > > > > > > > > > > adjust the limits to avoid that. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > > > > > > > > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Any interest in using igt to test this stuff? We > > > > > > > > > > > > > > have at least a bunch of > > > > > > > > > > > > > > tests already in there that try all k
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Mon, Oct 29, 2018 at 09:41:36AM +0100, Boris Brezillon wrote: > On Mon, 29 Oct 2018 09:06:40 +0100 > Daniel Vetter wrote: > > > On Fri, Oct 26, 2018 at 06:10:03PM +0300, Ville Syrjälä wrote: > > > On Fri, Oct 26, 2018 at 04:52:33PM +0200, Boris Brezillon wrote: > > > > On Fri, 26 Oct 2018 16:26:03 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Fri, Oct 26, 2018 at 3:57 PM Boris Brezillon > > > > > wrote: > > > > > > > > > > > > On Fri, 26 Oct 2018 15:30:26 +0200 > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > On Thu, Oct 25, 2018 at 11:41:14AM +0200, Boris Brezillon wrote: > > > > > > > > > > > > > > > On Thu, 25 Oct 2018 11:33:14 +0200 > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > On Thu, Oct 25, 2018 at 10:09:31AM +0200, Boris Brezillon > > > > > > > > > wrote: > > > > > > > > > > On Tue, 23 Oct 2018 15:44:43 +0200 > > > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > > > > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon > > > > > > > > > > > wrote: > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > > > > > > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > > > > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris > > > > > > > > > > > > > Brezillon wrote: > > > > > > > > > > > > > > The HVS block is supposed to fill the pixelvalve > > > > > > > > > > > > > > FIFOs fast enough to > > > > > > > > > > > > > > meet the requested framerate. The problem is, the > > > > > > > > > > > > > > HVS and memory bus > > > > > > > > > > > > > > bandwidths are limited, and if we don't take these > > > > > > > > > > > > > > limitations into > > > > > > > > > > > > > > account we might end up with HVS underflow errors. > > > > > > > > > > > > > > > > > > > > > > > > > > > > This patch is trying to model the per-plane HVS and > > > > > > > > > > > > > > memory bus bandwidth > > > > > > > > > > > > > > consumption and take a decision at atomic_check() > > > > > > > > > > > > > > time whether the > > > > > > > > > > > > > > estimated load will fit in the HVS and membus > > > > > > > > > > > > > > budget. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Note that we take an extra margin on the memory bus > > > > > > > > > > > > > > consumption to let > > > > > > > > > > > > > > the system run smoothly when other blocks are doing > > > > > > > > > > > > > > heavy use of the > > > > > > > > > > > > > > memory bus. Same goes for the HVS limit, except the > > > > > > > > > > > > > > margin is smaller in > > > > > > > > > > > > > > this case, since the HVS is not used by external > > > > > > > > > > > > > > components. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > This logic has been validated using a simple shell > > > > > > > > > > > > > > script and > > > > > > > > > > > > > > some instrumentation in the VC4 driver: > > > > > > > > > > > > > > > > > > > > > > > > > > > > - capture underflow errors at the HVS level and > > > > > > > > > > > > > > expose a debugfs file > > > > > > > > > > > > > > reporting those errors > > > > > > > > > > > > > > - add debugfs files to expose when atomic_check > > > > > > > > > > > > > > fails because of the > > > > > > > > > > > > > > HVS or membus load limitation or when it fails > > > > > > > > > > > > > > for other reasons > > > > > > > > > > > > > > > > > > > > > > > > > > > > The branch containing those modification is > > > > > > > > > > > > > > available here [1], and the > > > > > > > > > > > > > > script (which is internally using modetest) is here > > > > > > > > > > > > > > [2] (please note > > > > > > > > > > > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > > > > > > > > > > > > > > > > > > > > > Note that those modification tend to over-estimate > > > > > > > > > > > > > > the load, and thus > > > > > > > > > > > > > > reject setups that might have previously worked, so > > > > > > > > > > > > > > we might want to > > > > > > > > > > > > > > adjust the limits to avoid that. > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > > > > > > > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Any interest in using igt to test this stuff? We have > > > > > > > > > > > > > at least a bunch of > > > > > > > > > > > > > tests already in there that try all kinds of plane > > > > > > > > > > > > > setups. And we use > > > > > > > > > > > > > those to hunt for underruns on i915 hw. > > > > > > > > > > > > > > > > > > > > > > > > > > Wrt underrun reporting: On i915 we just dump them > > > > > > > > > > > > > into dmesg at t
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Mon, 29 Oct 2018 09:06:40 +0100 Daniel Vetter wrote: > On Fri, Oct 26, 2018 at 06:10:03PM +0300, Ville Syrjälä wrote: > > On Fri, Oct 26, 2018 at 04:52:33PM +0200, Boris Brezillon wrote: > > > On Fri, 26 Oct 2018 16:26:03 +0200 > > > Daniel Vetter wrote: > > > > > > > On Fri, Oct 26, 2018 at 3:57 PM Boris Brezillon > > > > wrote: > > > > > > > > > > On Fri, 26 Oct 2018 15:30:26 +0200 > > > > > Daniel Vetter wrote: > > > > > > > > > > > On Thu, Oct 25, 2018 at 11:41:14AM +0200, Boris Brezillon wrote: > > > > > > > On Thu, 25 Oct 2018 11:33:14 +0200 > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > On Thu, Oct 25, 2018 at 10:09:31AM +0200, Boris Brezillon > > > > > > > > wrote: > > > > > > > > > On Tue, 23 Oct 2018 15:44:43 +0200 > > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon > > > > > > > > > > wrote: > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > > > > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > > > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris > > > > > > > > > > > > Brezillon wrote: > > > > > > > > > > > > > The HVS block is supposed to fill the pixelvalve > > > > > > > > > > > > > FIFOs fast enough to > > > > > > > > > > > > > meet the requested framerate. The problem is, the HVS > > > > > > > > > > > > > and memory bus > > > > > > > > > > > > > bandwidths are limited, and if we don't take these > > > > > > > > > > > > > limitations into > > > > > > > > > > > > > account we might end up with HVS underflow errors. > > > > > > > > > > > > > > > > > > > > > > > > > > This patch is trying to model the per-plane HVS and > > > > > > > > > > > > > memory bus bandwidth > > > > > > > > > > > > > consumption and take a decision at atomic_check() > > > > > > > > > > > > > time whether the > > > > > > > > > > > > > estimated load will fit in the HVS and membus budget. > > > > > > > > > > > > > > > > > > > > > > > > > > Note that we take an extra margin on the memory bus > > > > > > > > > > > > > consumption to let > > > > > > > > > > > > > the system run smoothly when other blocks are doing > > > > > > > > > > > > > heavy use of the > > > > > > > > > > > > > memory bus. Same goes for the HVS limit, except the > > > > > > > > > > > > > margin is smaller in > > > > > > > > > > > > > this case, since the HVS is not used by external > > > > > > > > > > > > > components. > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > This logic has been validated using a simple shell > > > > > > > > > > > > > script and > > > > > > > > > > > > > some instrumentation in the VC4 driver: > > > > > > > > > > > > > > > > > > > > > > > > > > - capture underflow errors at the HVS level and > > > > > > > > > > > > > expose a debugfs file > > > > > > > > > > > > > reporting those errors > > > > > > > > > > > > > - add debugfs files to expose when atomic_check fails > > > > > > > > > > > > > because of the > > > > > > > > > > > > > HVS or membus load limitation or when it fails for > > > > > > > > > > > > > other reasons > > > > > > > > > > > > > > > > > > > > > > > > > > The branch containing those modification is available > > > > > > > > > > > > > here [1], and the > > > > > > > > > > > > > script (which is internally using modetest) is here > > > > > > > > > > > > > [2] (please note > > > > > > > > > > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > > > > > > > > > > > > > > > > > > > Note that those modification tend to over-estimate > > > > > > > > > > > > > the load, and thus > > > > > > > > > > > > > reject setups that might have previously worked, so > > > > > > > > > > > > > we might want to > > > > > > > > > > > > > adjust the limits to avoid that. > > > > > > > > > > > > > > > > > > > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > > > > > > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Any interest in using igt to test this stuff? We have > > > > > > > > > > > > at least a bunch of > > > > > > > > > > > > tests already in there that try all kinds of plane > > > > > > > > > > > > setups. And we use > > > > > > > > > > > > those to hunt for underruns on i915 hw. > > > > > > > > > > > > > > > > > > > > > > > > Wrt underrun reporting: On i915 we just dump them into > > > > > > > > > > > > dmesg at the error > > > > > > > > > > > > level, using DRM_ERROR, > > > > > > > > > > > > > > > > > > > > > > Are you masking the underrun interrupt after it's been > > > > > > > > > > > reported? If we > > > > > > > > > > > don't do that on VC4 we just end up flooding the > > > > > > > > > > > kern
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Fri, Oct 26, 2018 at 06:10:03PM +0300, Ville Syrjälä wrote: > On Fri, Oct 26, 2018 at 04:52:33PM +0200, Boris Brezillon wrote: > > On Fri, 26 Oct 2018 16:26:03 +0200 > > Daniel Vetter wrote: > > > > > On Fri, Oct 26, 2018 at 3:57 PM Boris Brezillon > > > wrote: > > > > > > > > On Fri, 26 Oct 2018 15:30:26 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Thu, Oct 25, 2018 at 11:41:14AM +0200, Boris Brezillon wrote: > > > > > > On Thu, 25 Oct 2018 11:33:14 +0200 > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > On Thu, Oct 25, 2018 at 10:09:31AM +0200, Boris Brezillon wrote: > > > > > > > > On Tue, 23 Oct 2018 15:44:43 +0200 > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon > > > > > > > > > wrote: > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon > > > > > > > > > > > wrote: > > > > > > > > > > > > The HVS block is supposed to fill the pixelvalve FIFOs > > > > > > > > > > > > fast enough to > > > > > > > > > > > > meet the requested framerate. The problem is, the HVS > > > > > > > > > > > > and memory bus > > > > > > > > > > > > bandwidths are limited, and if we don't take these > > > > > > > > > > > > limitations into > > > > > > > > > > > > account we might end up with HVS underflow errors. > > > > > > > > > > > > > > > > > > > > > > > > This patch is trying to model the per-plane HVS and > > > > > > > > > > > > memory bus bandwidth > > > > > > > > > > > > consumption and take a decision at atomic_check() time > > > > > > > > > > > > whether the > > > > > > > > > > > > estimated load will fit in the HVS and membus budget. > > > > > > > > > > > > > > > > > > > > > > > > Note that we take an extra margin on the memory bus > > > > > > > > > > > > consumption to let > > > > > > > > > > > > the system run smoothly when other blocks are doing > > > > > > > > > > > > heavy use of the > > > > > > > > > > > > memory bus. Same goes for the HVS limit, except the > > > > > > > > > > > > margin is smaller in > > > > > > > > > > > > this case, since the HVS is not used by external > > > > > > > > > > > > components. > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > This logic has been validated using a simple shell > > > > > > > > > > > > script and > > > > > > > > > > > > some instrumentation in the VC4 driver: > > > > > > > > > > > > > > > > > > > > > > > > - capture underflow errors at the HVS level and expose > > > > > > > > > > > > a debugfs file > > > > > > > > > > > > reporting those errors > > > > > > > > > > > > - add debugfs files to expose when atomic_check fails > > > > > > > > > > > > because of the > > > > > > > > > > > > HVS or membus load limitation or when it fails for > > > > > > > > > > > > other reasons > > > > > > > > > > > > > > > > > > > > > > > > The branch containing those modification is available > > > > > > > > > > > > here [1], and the > > > > > > > > > > > > script (which is internally using modetest) is here [2] > > > > > > > > > > > > (please note > > > > > > > > > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > > > > > > > > > > > > > > > > > Note that those modification tend to over-estimate the > > > > > > > > > > > > load, and thus > > > > > > > > > > > > reject setups that might have previously worked, so we > > > > > > > > > > > > might want to > > > > > > > > > > > > adjust the limits to avoid that. > > > > > > > > > > > > > > > > > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > > > > > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Any interest in using igt to test this stuff? We have at > > > > > > > > > > > least a bunch of > > > > > > > > > > > tests already in there that try all kinds of plane > > > > > > > > > > > setups. And we use > > > > > > > > > > > those to hunt for underruns on i915 hw. > > > > > > > > > > > > > > > > > > > > > > Wrt underrun reporting: On i915 we just dump them into > > > > > > > > > > > dmesg at the error > > > > > > > > > > > level, using DRM_ERROR, > > > > > > > > > > > > > > > > > > > > Are you masking the underrun interrupt after it's been > > > > > > > > > > reported? If we > > > > > > > > > > don't do that on VC4 we just end up flooding the kernel-log > > > > > > > > > > buffer until > > > > > > > > > > someone comes and update the config. > > > > > > > > > > > > > > > > > > Yeah we do that too. Rule is that a full modeset will clear > > > > > > > > > any underrun > > > > > > > > > masking (so tests need to make sure they start with a > > > > > > >
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Fri, Oct 26, 2018 at 04:52:33PM +0200, Boris Brezillon wrote: > On Fri, 26 Oct 2018 16:26:03 +0200 > Daniel Vetter wrote: > > > On Fri, Oct 26, 2018 at 3:57 PM Boris Brezillon > > wrote: > > > > > > On Fri, 26 Oct 2018 15:30:26 +0200 > > > Daniel Vetter wrote: > > > > > > > On Thu, Oct 25, 2018 at 11:41:14AM +0200, Boris Brezillon wrote: > > > > > On Thu, 25 Oct 2018 11:33:14 +0200 > > > > > Daniel Vetter wrote: > > > > > > > > > > > On Thu, Oct 25, 2018 at 10:09:31AM +0200, Boris Brezillon wrote: > > > > > > > On Tue, 23 Oct 2018 15:44:43 +0200 > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon > > > > > > > > wrote: > > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon > > > > > > > > > > wrote: > > > > > > > > > > > The HVS block is supposed to fill the pixelvalve FIFOs > > > > > > > > > > > fast enough to > > > > > > > > > > > meet the requested framerate. The problem is, the HVS and > > > > > > > > > > > memory bus > > > > > > > > > > > bandwidths are limited, and if we don't take these > > > > > > > > > > > limitations into > > > > > > > > > > > account we might end up with HVS underflow errors. > > > > > > > > > > > > > > > > > > > > > > This patch is trying to model the per-plane HVS and > > > > > > > > > > > memory bus bandwidth > > > > > > > > > > > consumption and take a decision at atomic_check() time > > > > > > > > > > > whether the > > > > > > > > > > > estimated load will fit in the HVS and membus budget. > > > > > > > > > > > > > > > > > > > > > > Note that we take an extra margin on the memory bus > > > > > > > > > > > consumption to let > > > > > > > > > > > the system run smoothly when other blocks are doing heavy > > > > > > > > > > > use of the > > > > > > > > > > > memory bus. Same goes for the HVS limit, except the > > > > > > > > > > > margin is smaller in > > > > > > > > > > > this case, since the HVS is not used by external > > > > > > > > > > > components. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > This logic has been validated using a simple shell script > > > > > > > > > > > and > > > > > > > > > > > some instrumentation in the VC4 driver: > > > > > > > > > > > > > > > > > > > > > > - capture underflow errors at the HVS level and expose a > > > > > > > > > > > debugfs file > > > > > > > > > > > reporting those errors > > > > > > > > > > > - add debugfs files to expose when atomic_check fails > > > > > > > > > > > because of the > > > > > > > > > > > HVS or membus load limitation or when it fails for > > > > > > > > > > > other reasons > > > > > > > > > > > > > > > > > > > > > > The branch containing those modification is available > > > > > > > > > > > here [1], and the > > > > > > > > > > > script (which is internally using modetest) is here [2] > > > > > > > > > > > (please note > > > > > > > > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > > > > > > > > > > > > > > > Note that those modification tend to over-estimate the > > > > > > > > > > > load, and thus > > > > > > > > > > > reject setups that might have previously worked, so we > > > > > > > > > > > might want to > > > > > > > > > > > adjust the limits to avoid that. > > > > > > > > > > > > > > > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > > > > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > > > > > > > > > > > > > > > Any interest in using igt to test this stuff? We have at > > > > > > > > > > least a bunch of > > > > > > > > > > tests already in there that try all kinds of plane setups. > > > > > > > > > > And we use > > > > > > > > > > those to hunt for underruns on i915 hw. > > > > > > > > > > > > > > > > > > > > Wrt underrun reporting: On i915 we just dump them into > > > > > > > > > > dmesg at the error > > > > > > > > > > level, using DRM_ERROR, > > > > > > > > > > > > > > > > > > Are you masking the underrun interrupt after it's been > > > > > > > > > reported? If we > > > > > > > > > don't do that on VC4 we just end up flooding the kernel-log > > > > > > > > > buffer until > > > > > > > > > someone comes and update the config. > > > > > > > > > > > > > > > > Yeah we do that too. Rule is that a full modeset will clear any > > > > > > > > underrun > > > > > > > > masking (so tests need to make sure they start with a modeset, > > > > > > > > or it'll be > > > > > > > > for nothing). > > > > > > > > > > > > > > > > > > > > > > > > > > > plus a tracepoint. See e.g. > > > > > > > > > > intel_pch_fifo_underrun_irq_handler(). If there's interest > > > > > > > > > > we could > > > > > > > > > > perhaps extract t
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Fri, 26 Oct 2018 16:26:03 +0200 Daniel Vetter wrote: > On Fri, Oct 26, 2018 at 3:57 PM Boris Brezillon > wrote: > > > > On Fri, 26 Oct 2018 15:30:26 +0200 > > Daniel Vetter wrote: > > > > > On Thu, Oct 25, 2018 at 11:41:14AM +0200, Boris Brezillon wrote: > > > > On Thu, 25 Oct 2018 11:33:14 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Thu, Oct 25, 2018 at 10:09:31AM +0200, Boris Brezillon wrote: > > > > > > On Tue, 23 Oct 2018 15:44:43 +0200 > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon wrote: > > > > > > > > Hi Daniel, > > > > > > > > > > > > > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon > > > > > > > > > wrote: > > > > > > > > > > The HVS block is supposed to fill the pixelvalve FIFOs fast > > > > > > > > > > enough to > > > > > > > > > > meet the requested framerate. The problem is, the HVS and > > > > > > > > > > memory bus > > > > > > > > > > bandwidths are limited, and if we don't take these > > > > > > > > > > limitations into > > > > > > > > > > account we might end up with HVS underflow errors. > > > > > > > > > > > > > > > > > > > > This patch is trying to model the per-plane HVS and memory > > > > > > > > > > bus bandwidth > > > > > > > > > > consumption and take a decision at atomic_check() time > > > > > > > > > > whether the > > > > > > > > > > estimated load will fit in the HVS and membus budget. > > > > > > > > > > > > > > > > > > > > Note that we take an extra margin on the memory bus > > > > > > > > > > consumption to let > > > > > > > > > > the system run smoothly when other blocks are doing heavy > > > > > > > > > > use of the > > > > > > > > > > memory bus. Same goes for the HVS limit, except the margin > > > > > > > > > > is smaller in > > > > > > > > > > this case, since the HVS is not used by external components. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > > > > > --- > > > > > > > > > > This logic has been validated using a simple shell script > > > > > > > > > > and > > > > > > > > > > some instrumentation in the VC4 driver: > > > > > > > > > > > > > > > > > > > > - capture underflow errors at the HVS level and expose a > > > > > > > > > > debugfs file > > > > > > > > > > reporting those errors > > > > > > > > > > - add debugfs files to expose when atomic_check fails > > > > > > > > > > because of the > > > > > > > > > > HVS or membus load limitation or when it fails for other > > > > > > > > > > reasons > > > > > > > > > > > > > > > > > > > > The branch containing those modification is available here > > > > > > > > > > [1], and the > > > > > > > > > > script (which is internally using modetest) is here [2] > > > > > > > > > > (please note > > > > > > > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > > > > > > > > > > > > > Note that those modification tend to over-estimate the > > > > > > > > > > load, and thus > > > > > > > > > > reject setups that might have previously worked, so we > > > > > > > > > > might want to > > > > > > > > > > adjust the limits to avoid that. > > > > > > > > > > > > > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > > > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > > > > > > > > > > > > > Any interest in using igt to test this stuff? We have at > > > > > > > > > least a bunch of > > > > > > > > > tests already in there that try all kinds of plane setups. > > > > > > > > > And we use > > > > > > > > > those to hunt for underruns on i915 hw. > > > > > > > > > > > > > > > > > > Wrt underrun reporting: On i915 we just dump them into dmesg > > > > > > > > > at the error > > > > > > > > > level, using DRM_ERROR, > > > > > > > > > > > > > > > > Are you masking the underrun interrupt after it's been > > > > > > > > reported? If we > > > > > > > > don't do that on VC4 we just end up flooding the kernel-log > > > > > > > > buffer until > > > > > > > > someone comes and update the config. > > > > > > > > > > > > > > Yeah we do that too. Rule is that a full modeset will clear any > > > > > > > underrun > > > > > > > masking (so tests need to make sure they start with a modeset, or > > > > > > > it'll be > > > > > > > for nothing). > > > > > > > > > > > > > > > > > > > > > > > > plus a tracepoint. See e.g. > > > > > > > > > intel_pch_fifo_underrun_irq_handler(). If there's interest we > > > > > > > > > could > > > > > > > > > perhaps extract this into something common, similar to what > > > > > > > > > was done with > > > > > > > > > crc support already. > > > > > > > > > > > > > > > > > > > > > > > > > I'm not a big fan of hardcoded trace points in general (because > > > > > > > > of the > > > > > > > > whole "it's part of the stable ABI" thing), and in this case,
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Fri, Oct 26, 2018 at 3:57 PM Boris Brezillon wrote: > > On Fri, 26 Oct 2018 15:30:26 +0200 > Daniel Vetter wrote: > > > On Thu, Oct 25, 2018 at 11:41:14AM +0200, Boris Brezillon wrote: > > > On Thu, 25 Oct 2018 11:33:14 +0200 > > > Daniel Vetter wrote: > > > > > > > On Thu, Oct 25, 2018 at 10:09:31AM +0200, Boris Brezillon wrote: > > > > > On Tue, 23 Oct 2018 15:44:43 +0200 > > > > > Daniel Vetter wrote: > > > > > > > > > > > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon wrote: > > > > > > > Hi Daniel, > > > > > > > > > > > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote: > > > > > > > > > The HVS block is supposed to fill the pixelvalve FIFOs fast > > > > > > > > > enough to > > > > > > > > > meet the requested framerate. The problem is, the HVS and > > > > > > > > > memory bus > > > > > > > > > bandwidths are limited, and if we don't take these > > > > > > > > > limitations into > > > > > > > > > account we might end up with HVS underflow errors. > > > > > > > > > > > > > > > > > > This patch is trying to model the per-plane HVS and memory > > > > > > > > > bus bandwidth > > > > > > > > > consumption and take a decision at atomic_check() time > > > > > > > > > whether the > > > > > > > > > estimated load will fit in the HVS and membus budget. > > > > > > > > > > > > > > > > > > Note that we take an extra margin on the memory bus > > > > > > > > > consumption to let > > > > > > > > > the system run smoothly when other blocks are doing heavy use > > > > > > > > > of the > > > > > > > > > memory bus. Same goes for the HVS limit, except the margin is > > > > > > > > > smaller in > > > > > > > > > this case, since the HVS is not used by external components. > > > > > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > > > > --- > > > > > > > > > This logic has been validated using a simple shell script and > > > > > > > > > some instrumentation in the VC4 driver: > > > > > > > > > > > > > > > > > > - capture underflow errors at the HVS level and expose a > > > > > > > > > debugfs file > > > > > > > > > reporting those errors > > > > > > > > > - add debugfs files to expose when atomic_check fails because > > > > > > > > > of the > > > > > > > > > HVS or membus load limitation or when it fails for other > > > > > > > > > reasons > > > > > > > > > > > > > > > > > > The branch containing those modification is available here > > > > > > > > > [1], and the > > > > > > > > > script (which is internally using modetest) is here [2] > > > > > > > > > (please note > > > > > > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > > > > > > > > > > > Note that those modification tend to over-estimate the load, > > > > > > > > > and thus > > > > > > > > > reject setups that might have previously worked, so we might > > > > > > > > > want to > > > > > > > > > adjust the limits to avoid that. > > > > > > > > > > > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > > > > > > > > > > > Any interest in using igt to test this stuff? We have at least > > > > > > > > a bunch of > > > > > > > > tests already in there that try all kinds of plane setups. And > > > > > > > > we use > > > > > > > > those to hunt for underruns on i915 hw. > > > > > > > > > > > > > > > > Wrt underrun reporting: On i915 we just dump them into dmesg at > > > > > > > > the error > > > > > > > > level, using DRM_ERROR, > > > > > > > > > > > > > > Are you masking the underrun interrupt after it's been reported? > > > > > > > If we > > > > > > > don't do that on VC4 we just end up flooding the kernel-log > > > > > > > buffer until > > > > > > > someone comes and update the config. > > > > > > > > > > > > Yeah we do that too. Rule is that a full modeset will clear any > > > > > > underrun > > > > > > masking (so tests need to make sure they start with a modeset, or > > > > > > it'll be > > > > > > for nothing). > > > > > > > > > > > > > > > > > > > > > plus a tracepoint. See e.g. > > > > > > > > intel_pch_fifo_underrun_irq_handler(). If there's interest we > > > > > > > > could > > > > > > > > perhaps extract this into something common, similar to what was > > > > > > > > done with > > > > > > > > crc support already. > > > > > > > > > > > > > > > > > > > > > > I'm not a big fan of hardcoded trace points in general (because > > > > > > > of the > > > > > > > whole "it's part of the stable ABI" thing), and in this case, > > > > > > > making the > > > > > > > tracepoint generic sounds even more risky to me. Indeed, how can > > > > > > > we know > > > > > > > about all the HW specific bits one might want to expose. For > > > > > > > instance, > > > > > > > I see the intel underrun tracepoint exposes a struct with a frame > > > > > > > and > > > > >
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Fri, 26 Oct 2018 15:30:26 +0200 Daniel Vetter wrote: > On Thu, Oct 25, 2018 at 11:41:14AM +0200, Boris Brezillon wrote: > > On Thu, 25 Oct 2018 11:33:14 +0200 > > Daniel Vetter wrote: > > > > > On Thu, Oct 25, 2018 at 10:09:31AM +0200, Boris Brezillon wrote: > > > > On Tue, 23 Oct 2018 15:44:43 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon wrote: > > > > > > Hi Daniel, > > > > > > > > > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote: > > > > > > > > > > > > > > > The HVS block is supposed to fill the pixelvalve FIFOs fast > > > > > > > > enough to > > > > > > > > meet the requested framerate. The problem is, the HVS and > > > > > > > > memory bus > > > > > > > > bandwidths are limited, and if we don't take these limitations > > > > > > > > into > > > > > > > > account we might end up with HVS underflow errors. > > > > > > > > > > > > > > > > This patch is trying to model the per-plane HVS and memory bus > > > > > > > > bandwidth > > > > > > > > consumption and take a decision at atomic_check() time whether > > > > > > > > the > > > > > > > > estimated load will fit in the HVS and membus budget. > > > > > > > > > > > > > > > > Note that we take an extra margin on the memory bus consumption > > > > > > > > to let > > > > > > > > the system run smoothly when other blocks are doing heavy use > > > > > > > > of the > > > > > > > > memory bus. Same goes for the HVS limit, except the margin is > > > > > > > > smaller in > > > > > > > > this case, since the HVS is not used by external components. > > > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > > > --- > > > > > > > > This logic has been validated using a simple shell script and > > > > > > > > some instrumentation in the VC4 driver: > > > > > > > > > > > > > > > > - capture underflow errors at the HVS level and expose a > > > > > > > > debugfs file > > > > > > > > reporting those errors > > > > > > > > - add debugfs files to expose when atomic_check fails because > > > > > > > > of the > > > > > > > > HVS or membus load limitation or when it fails for other > > > > > > > > reasons > > > > > > > > > > > > > > > > The branch containing those modification is available here [1], > > > > > > > > and the > > > > > > > > script (which is internally using modetest) is here [2] (please > > > > > > > > note > > > > > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > > > > > > > > > Note that those modification tend to over-estimate the load, > > > > > > > > and thus > > > > > > > > reject setups that might have previously worked, so we might > > > > > > > > want to > > > > > > > > adjust the limits to avoid that. > > > > > > > > > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > > > > > > > > > Any interest in using igt to test this stuff? We have at least a > > > > > > > bunch of > > > > > > > tests already in there that try all kinds of plane setups. And we > > > > > > > use > > > > > > > those to hunt for underruns on i915 hw. > > > > > > > > > > > > > > Wrt underrun reporting: On i915 we just dump them into dmesg at > > > > > > > the error > > > > > > > level, using DRM_ERROR, > > > > > > > > > > > > Are you masking the underrun interrupt after it's been reported? If > > > > > > we > > > > > > don't do that on VC4 we just end up flooding the kernel-log buffer > > > > > > until > > > > > > someone comes and update the config. > > > > > > > > > > Yeah we do that too. Rule is that a full modeset will clear any > > > > > underrun > > > > > masking (so tests need to make sure they start with a modeset, or > > > > > it'll be > > > > > for nothing). > > > > > > > > > > > > > > > > > > plus a tracepoint. See e.g. > > > > > > > intel_pch_fifo_underrun_irq_handler(). If there's interest we > > > > > > > could > > > > > > > perhaps extract this into something common, similar to what was > > > > > > > done with > > > > > > > crc support already. > > > > > > > > > > > > > > > > > > > I'm not a big fan of hardcoded trace points in general (because of > > > > > > the > > > > > > whole "it's part of the stable ABI" thing), and in this case, > > > > > > making the > > > > > > tracepoint generic sounds even more risky to me. Indeed, how can we > > > > > > know > > > > > > about all the HW specific bits one might want to expose. For > > > > > > instance, > > > > > > I see the intel underrun tracepoint exposes a struct with a frame > > > > > > and > > > > > > scanline field, and AFAICT, we don't have such information in the > > > > > > VC4 > > > > > > case. > > > > > > > > > > > > Any opinion on that? > > > > > > > > > > It's
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Thu, Oct 25, 2018 at 11:41:14AM +0200, Boris Brezillon wrote: > On Thu, 25 Oct 2018 11:33:14 +0200 > Daniel Vetter wrote: > > > On Thu, Oct 25, 2018 at 10:09:31AM +0200, Boris Brezillon wrote: > > > On Tue, 23 Oct 2018 15:44:43 +0200 > > > Daniel Vetter wrote: > > > > > > > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon wrote: > > > > > Hi Daniel, > > > > > > > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > > > > Daniel Vetter wrote: > > > > > > > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote: > > > > > > > The HVS block is supposed to fill the pixelvalve FIFOs fast > > > > > > > enough to > > > > > > > meet the requested framerate. The problem is, the HVS and memory > > > > > > > bus > > > > > > > bandwidths are limited, and if we don't take these limitations > > > > > > > into > > > > > > > account we might end up with HVS underflow errors. > > > > > > > > > > > > > > This patch is trying to model the per-plane HVS and memory bus > > > > > > > bandwidth > > > > > > > consumption and take a decision at atomic_check() time whether the > > > > > > > estimated load will fit in the HVS and membus budget. > > > > > > > > > > > > > > Note that we take an extra margin on the memory bus consumption > > > > > > > to let > > > > > > > the system run smoothly when other blocks are doing heavy use of > > > > > > > the > > > > > > > memory bus. Same goes for the HVS limit, except the margin is > > > > > > > smaller in > > > > > > > this case, since the HVS is not used by external components. > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > > --- > > > > > > > This logic has been validated using a simple shell script and > > > > > > > some instrumentation in the VC4 driver: > > > > > > > > > > > > > > - capture underflow errors at the HVS level and expose a debugfs > > > > > > > file > > > > > > > reporting those errors > > > > > > > - add debugfs files to expose when atomic_check fails because of > > > > > > > the > > > > > > > HVS or membus load limitation or when it fails for other reasons > > > > > > > > > > > > > > The branch containing those modification is available here [1], > > > > > > > and the > > > > > > > script (which is internally using modetest) is here [2] (please > > > > > > > note > > > > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > > > > > > > Note that those modification tend to over-estimate the load, and > > > > > > > thus > > > > > > > reject setups that might have previously worked, so we might want > > > > > > > to > > > > > > > adjust the limits to avoid that. > > > > > > > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > > > > > > > Any interest in using igt to test this stuff? We have at least a > > > > > > bunch of > > > > > > tests already in there that try all kinds of plane setups. And we > > > > > > use > > > > > > those to hunt for underruns on i915 hw. > > > > > > > > > > > > Wrt underrun reporting: On i915 we just dump them into dmesg at the > > > > > > error > > > > > > level, using DRM_ERROR, > > > > > > > > > > Are you masking the underrun interrupt after it's been reported? If we > > > > > don't do that on VC4 we just end up flooding the kernel-log buffer > > > > > until > > > > > someone comes and update the config. > > > > > > > > Yeah we do that too. Rule is that a full modeset will clear any underrun > > > > masking (so tests need to make sure they start with a modeset, or it'll > > > > be > > > > for nothing). > > > > > > > > > > > > > > > plus a tracepoint. See e.g. > > > > > > intel_pch_fifo_underrun_irq_handler(). If there's interest we could > > > > > > perhaps extract this into something common, similar to what was > > > > > > done with > > > > > > crc support already. > > > > > > > > > > > > > > > > I'm not a big fan of hardcoded trace points in general (because of the > > > > > whole "it's part of the stable ABI" thing), and in this case, making > > > > > the > > > > > tracepoint generic sounds even more risky to me. Indeed, how can we > > > > > know > > > > > about all the HW specific bits one might want to expose. For instance, > > > > > I see the intel underrun tracepoint exposes a struct with a frame and > > > > > scanline field, and AFAICT, we don't have such information in the VC4 > > > > > case. > > > > > > > > > > Any opinion on that? > > > > > > > > It's only abi if you're unlucky. If it's just for debugging and > > > > validation, you can change it again. Tbh, no idea why we even have these > > > > tracepoints, they're fairly useless imo. CI only relies upon the dmesg > > > > output. Maybe run git blame and ask the original author, we can probably > > > > update them to suit our needs. > > > > > > Okay, I think I'll go for a generic debugfs entry that returns true > > > when
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Thu, 25 Oct 2018 11:33:14 +0200 Daniel Vetter wrote: > On Thu, Oct 25, 2018 at 10:09:31AM +0200, Boris Brezillon wrote: > > On Tue, 23 Oct 2018 15:44:43 +0200 > > Daniel Vetter wrote: > > > > > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon wrote: > > > > Hi Daniel, > > > > > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > > > Daniel Vetter wrote: > > > > > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote: > > > > > > The HVS block is supposed to fill the pixelvalve FIFOs fast enough > > > > > > to > > > > > > meet the requested framerate. The problem is, the HVS and memory bus > > > > > > bandwidths are limited, and if we don't take these limitations into > > > > > > account we might end up with HVS underflow errors. > > > > > > > > > > > > This patch is trying to model the per-plane HVS and memory bus > > > > > > bandwidth > > > > > > consumption and take a decision at atomic_check() time whether the > > > > > > estimated load will fit in the HVS and membus budget. > > > > > > > > > > > > Note that we take an extra margin on the memory bus consumption to > > > > > > let > > > > > > the system run smoothly when other blocks are doing heavy use of the > > > > > > memory bus. Same goes for the HVS limit, except the margin is > > > > > > smaller in > > > > > > this case, since the HVS is not used by external components. > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > --- > > > > > > This logic has been validated using a simple shell script and > > > > > > some instrumentation in the VC4 driver: > > > > > > > > > > > > - capture underflow errors at the HVS level and expose a debugfs > > > > > > file > > > > > > reporting those errors > > > > > > - add debugfs files to expose when atomic_check fails because of the > > > > > > HVS or membus load limitation or when it fails for other reasons > > > > > > > > > > > > The branch containing those modification is available here [1], and > > > > > > the > > > > > > script (which is internally using modetest) is here [2] (please note > > > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > > > > > Note that those modification tend to over-estimate the load, and > > > > > > thus > > > > > > reject setups that might have previously worked, so we might want to > > > > > > adjust the limits to avoid that. > > > > > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > > > > > Any interest in using igt to test this stuff? We have at least a > > > > > bunch of > > > > > tests already in there that try all kinds of plane setups. And we use > > > > > those to hunt for underruns on i915 hw. > > > > > > > > > > Wrt underrun reporting: On i915 we just dump them into dmesg at the > > > > > error > > > > > level, using DRM_ERROR, > > > > > > > > Are you masking the underrun interrupt after it's been reported? If we > > > > don't do that on VC4 we just end up flooding the kernel-log buffer until > > > > someone comes and update the config. > > > > > > Yeah we do that too. Rule is that a full modeset will clear any underrun > > > masking (so tests need to make sure they start with a modeset, or it'll be > > > for nothing). > > > > > > > > > > > > plus a tracepoint. See e.g. > > > > > intel_pch_fifo_underrun_irq_handler(). If there's interest we could > > > > > perhaps extract this into something common, similar to what was done > > > > > with > > > > > crc support already. > > > > > > > > > > > > > I'm not a big fan of hardcoded trace points in general (because of the > > > > whole "it's part of the stable ABI" thing), and in this case, making the > > > > tracepoint generic sounds even more risky to me. Indeed, how can we know > > > > about all the HW specific bits one might want to expose. For instance, > > > > I see the intel underrun tracepoint exposes a struct with a frame and > > > > scanline field, and AFAICT, we don't have such information in the VC4 > > > > case. > > > > > > > > Any opinion on that? > > > > > > It's only abi if you're unlucky. If it's just for debugging and > > > validation, you can change it again. Tbh, no idea why we even have these > > > tracepoints, they're fairly useless imo. CI only relies upon the dmesg > > > output. Maybe run git blame and ask the original author, we can probably > > > update them to suit our needs. > > > > Okay, I think I'll go for a generic debugfs entry that returns true > > when an underrun error happened since the last modeset, false otherwise. > > > > Next question is: should I attach the underrun status to the drm_device > > or have one per CRTC? In my case, I only care about the "has an > > underrun error occurred on any of the active CRTC" case, so I'd vote for > > a per-device underrun status. > > Yeah probably good enough. For our CI all we care about is the warn/erro
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Thu, Oct 25, 2018 at 10:09:31AM +0200, Boris Brezillon wrote: > On Tue, 23 Oct 2018 15:44:43 +0200 > Daniel Vetter wrote: > > > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon wrote: > > > Hi Daniel, > > > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > > Daniel Vetter wrote: > > > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote: > > > > > The HVS block is supposed to fill the pixelvalve FIFOs fast enough to > > > > > meet the requested framerate. The problem is, the HVS and memory bus > > > > > bandwidths are limited, and if we don't take these limitations into > > > > > account we might end up with HVS underflow errors. > > > > > > > > > > This patch is trying to model the per-plane HVS and memory bus > > > > > bandwidth > > > > > consumption and take a decision at atomic_check() time whether the > > > > > estimated load will fit in the HVS and membus budget. > > > > > > > > > > Note that we take an extra margin on the memory bus consumption to let > > > > > the system run smoothly when other blocks are doing heavy use of the > > > > > memory bus. Same goes for the HVS limit, except the margin is smaller > > > > > in > > > > > this case, since the HVS is not used by external components. > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > --- > > > > > This logic has been validated using a simple shell script and > > > > > some instrumentation in the VC4 driver: > > > > > > > > > > - capture underflow errors at the HVS level and expose a debugfs file > > > > > reporting those errors > > > > > - add debugfs files to expose when atomic_check fails because of the > > > > > HVS or membus load limitation or when it fails for other reasons > > > > > > > > > > The branch containing those modification is available here [1], and > > > > > the > > > > > script (which is internally using modetest) is here [2] (please note > > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > > > Note that those modification tend to over-estimate the load, and thus > > > > > reject setups that might have previously worked, so we might want to > > > > > adjust the limits to avoid that. > > > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > > > Any interest in using igt to test this stuff? We have at least a bunch > > > > of > > > > tests already in there that try all kinds of plane setups. And we use > > > > those to hunt for underruns on i915 hw. > > > > > > > > Wrt underrun reporting: On i915 we just dump them into dmesg at the > > > > error > > > > level, using DRM_ERROR, > > > > > > Are you masking the underrun interrupt after it's been reported? If we > > > don't do that on VC4 we just end up flooding the kernel-log buffer until > > > someone comes and update the config. > > > > Yeah we do that too. Rule is that a full modeset will clear any underrun > > masking (so tests need to make sure they start with a modeset, or it'll be > > for nothing). > > > > > > > > > plus a tracepoint. See e.g. > > > > intel_pch_fifo_underrun_irq_handler(). If there's interest we could > > > > perhaps extract this into something common, similar to what was done > > > > with > > > > crc support already. > > > > > > > > > > I'm not a big fan of hardcoded trace points in general (because of the > > > whole "it's part of the stable ABI" thing), and in this case, making the > > > tracepoint generic sounds even more risky to me. Indeed, how can we know > > > about all the HW specific bits one might want to expose. For instance, > > > I see the intel underrun tracepoint exposes a struct with a frame and > > > scanline field, and AFAICT, we don't have such information in the VC4 > > > case. > > > > > > Any opinion on that? > > > > It's only abi if you're unlucky. If it's just for debugging and > > validation, you can change it again. Tbh, no idea why we even have these > > tracepoints, they're fairly useless imo. CI only relies upon the dmesg > > output. Maybe run git blame and ask the original author, we can probably > > update them to suit our needs. > > Okay, I think I'll go for a generic debugfs entry that returns true > when an underrun error happened since the last modeset, false otherwise. > > Next question is: should I attach the underrun status to the drm_device > or have one per CRTC? In my case, I only care about the "has an > underrun error occurred on any of the active CRTC" case, so I'd vote for > a per-device underrun status. Yeah probably good enough. For our CI all we care about is the warn/error level dmesg output. Anything at that level is considered a CI failure. What do you need the debugfs file for? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mail
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Tue, 23 Oct 2018 15:44:43 +0200 Daniel Vetter wrote: > On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon wrote: > > Hi Daniel, > > > > On Tue, 16 Oct 2018 14:57:43 +0200 > > Daniel Vetter wrote: > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote: > > > > The HVS block is supposed to fill the pixelvalve FIFOs fast enough to > > > > meet the requested framerate. The problem is, the HVS and memory bus > > > > bandwidths are limited, and if we don't take these limitations into > > > > account we might end up with HVS underflow errors. > > > > > > > > This patch is trying to model the per-plane HVS and memory bus bandwidth > > > > consumption and take a decision at atomic_check() time whether the > > > > estimated load will fit in the HVS and membus budget. > > > > > > > > Note that we take an extra margin on the memory bus consumption to let > > > > the system run smoothly when other blocks are doing heavy use of the > > > > memory bus. Same goes for the HVS limit, except the margin is smaller in > > > > this case, since the HVS is not used by external components. > > > > > > > > Signed-off-by: Boris Brezillon > > > > --- > > > > This logic has been validated using a simple shell script and > > > > some instrumentation in the VC4 driver: > > > > > > > > - capture underflow errors at the HVS level and expose a debugfs file > > > > reporting those errors > > > > - add debugfs files to expose when atomic_check fails because of the > > > > HVS or membus load limitation or when it fails for other reasons > > > > > > > > The branch containing those modification is available here [1], and the > > > > script (which is internally using modetest) is here [2] (please note > > > > that I'm bad at writing shell scripts :-)). > > > > > > > > Note that those modification tend to over-estimate the load, and thus > > > > reject setups that might have previously worked, so we might want to > > > > adjust the limits to avoid that. > > > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > > > Any interest in using igt to test this stuff? We have at least a bunch of > > > tests already in there that try all kinds of plane setups. And we use > > > those to hunt for underruns on i915 hw. > > > > > > Wrt underrun reporting: On i915 we just dump them into dmesg at the error > > > level, using DRM_ERROR, > > > > Are you masking the underrun interrupt after it's been reported? If we > > don't do that on VC4 we just end up flooding the kernel-log buffer until > > someone comes and update the config. > > Yeah we do that too. Rule is that a full modeset will clear any underrun > masking (so tests need to make sure they start with a modeset, or it'll be > for nothing). > > > > > > plus a tracepoint. See e.g. > > > intel_pch_fifo_underrun_irq_handler(). If there's interest we could > > > perhaps extract this into something common, similar to what was done with > > > crc support already. > > > > > > > I'm not a big fan of hardcoded trace points in general (because of the > > whole "it's part of the stable ABI" thing), and in this case, making the > > tracepoint generic sounds even more risky to me. Indeed, how can we know > > about all the HW specific bits one might want to expose. For instance, > > I see the intel underrun tracepoint exposes a struct with a frame and > > scanline field, and AFAICT, we don't have such information in the VC4 > > case. > > > > Any opinion on that? > > It's only abi if you're unlucky. If it's just for debugging and > validation, you can change it again. Tbh, no idea why we even have these > tracepoints, they're fairly useless imo. CI only relies upon the dmesg > output. Maybe run git blame and ask the original author, we can probably > update them to suit our needs. Okay, I think I'll go for a generic debugfs entry that returns true when an underrun error happened since the last modeset, false otherwise. Next question is: should I attach the underrun status to the drm_device or have one per CRTC? In my case, I only care about the "has an underrun error occurred on any of the active CRTC" case, so I'd vote for a per-device underrun status. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Tue, Oct 23, 2018 at 09:55:08AM +0200, Boris Brezillon wrote: > Hi Daniel, > > On Tue, 16 Oct 2018 14:57:43 +0200 > Daniel Vetter wrote: > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote: > > > The HVS block is supposed to fill the pixelvalve FIFOs fast enough to > > > meet the requested framerate. The problem is, the HVS and memory bus > > > bandwidths are limited, and if we don't take these limitations into > > > account we might end up with HVS underflow errors. > > > > > > This patch is trying to model the per-plane HVS and memory bus bandwidth > > > consumption and take a decision at atomic_check() time whether the > > > estimated load will fit in the HVS and membus budget. > > > > > > Note that we take an extra margin on the memory bus consumption to let > > > the system run smoothly when other blocks are doing heavy use of the > > > memory bus. Same goes for the HVS limit, except the margin is smaller in > > > this case, since the HVS is not used by external components. > > > > > > Signed-off-by: Boris Brezillon > > > --- > > > This logic has been validated using a simple shell script and > > > some instrumentation in the VC4 driver: > > > > > > - capture underflow errors at the HVS level and expose a debugfs file > > > reporting those errors > > > - add debugfs files to expose when atomic_check fails because of the > > > HVS or membus load limitation or when it fails for other reasons > > > > > > The branch containing those modification is available here [1], and the > > > script (which is internally using modetest) is here [2] (please note > > > that I'm bad at writing shell scripts :-)). > > > > > > Note that those modification tend to over-estimate the load, and thus > > > reject setups that might have previously worked, so we might want to > > > adjust the limits to avoid that. > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > > > Any interest in using igt to test this stuff? We have at least a bunch of > > tests already in there that try all kinds of plane setups. And we use > > those to hunt for underruns on i915 hw. > > > > Wrt underrun reporting: On i915 we just dump them into dmesg at the error > > level, using DRM_ERROR, > > Are you masking the underrun interrupt after it's been reported? If we > don't do that on VC4 we just end up flooding the kernel-log buffer until > someone comes and update the config. Yeah we do that too. Rule is that a full modeset will clear any underrun masking (so tests need to make sure they start with a modeset, or it'll be for nothing). > > > plus a tracepoint. See e.g. > > intel_pch_fifo_underrun_irq_handler(). If there's interest we could > > perhaps extract this into something common, similar to what was done with > > crc support already. > > > > I'm not a big fan of hardcoded trace points in general (because of the > whole "it's part of the stable ABI" thing), and in this case, making the > tracepoint generic sounds even more risky to me. Indeed, how can we know > about all the HW specific bits one might want to expose. For instance, > I see the intel underrun tracepoint exposes a struct with a frame and > scanline field, and AFAICT, we don't have such information in the VC4 > case. > > Any opinion on that? It's only abi if you're unlucky. If it's just for debugging and validation, you can change it again. Tbh, no idea why we even have these tracepoints, they're fairly useless imo. CI only relies upon the dmesg output. Maybe run git blame and ask the original author, we can probably update them to suit our needs. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
Hi Daniel, On Tue, 16 Oct 2018 14:57:43 +0200 Daniel Vetter wrote: > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote: > > The HVS block is supposed to fill the pixelvalve FIFOs fast enough to > > meet the requested framerate. The problem is, the HVS and memory bus > > bandwidths are limited, and if we don't take these limitations into > > account we might end up with HVS underflow errors. > > > > This patch is trying to model the per-plane HVS and memory bus bandwidth > > consumption and take a decision at atomic_check() time whether the > > estimated load will fit in the HVS and membus budget. > > > > Note that we take an extra margin on the memory bus consumption to let > > the system run smoothly when other blocks are doing heavy use of the > > memory bus. Same goes for the HVS limit, except the margin is smaller in > > this case, since the HVS is not used by external components. > > > > Signed-off-by: Boris Brezillon > > --- > > This logic has been validated using a simple shell script and > > some instrumentation in the VC4 driver: > > > > - capture underflow errors at the HVS level and expose a debugfs file > > reporting those errors > > - add debugfs files to expose when atomic_check fails because of the > > HVS or membus load limitation or when it fails for other reasons > > > > The branch containing those modification is available here [1], and the > > script (which is internally using modetest) is here [2] (please note > > that I'm bad at writing shell scripts :-)). > > > > Note that those modification tend to over-estimate the load, and thus > > reject setups that might have previously worked, so we might want to > > adjust the limits to avoid that. > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test > > Any interest in using igt to test this stuff? We have at least a bunch of > tests already in there that try all kinds of plane setups. And we use > those to hunt for underruns on i915 hw. > > Wrt underrun reporting: On i915 we just dump them into dmesg at the error > level, using DRM_ERROR, Are you masking the underrun interrupt after it's been reported? If we don't do that on VC4 we just end up flooding the kernel-log buffer until someone comes and update the config. > plus a tracepoint. See e.g. > intel_pch_fifo_underrun_irq_handler(). If there's interest we could > perhaps extract this into something common, similar to what was done with > crc support already. > I'm not a big fan of hardcoded trace points in general (because of the whole "it's part of the stable ABI" thing), and in this case, making the tracepoint generic sounds even more risky to me. Indeed, how can we know about all the HW specific bits one might want to expose. For instance, I see the intel underrun tracepoint exposes a struct with a frame and scanline field, and AFAICT, we don't have such information in the VC4 case. Any opinion on that? Regards, Boris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
+Rob
On Tue, 16 Oct 2018 18:41:51 +0200
Daniel Vetter wrote:
> On Tue, Oct 16, 2018 at 04:28:09PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 16, 2018 at 03:12:54PM +0200, Daniel Vetter wrote:
> > > On Tue, Oct 16, 2018 at 3:10 PM Boris Brezillon
> > > wrote:
> > > >
> > > > Hi Daniel,
> > > >
> > > > On Tue, 16 Oct 2018 14:57:43 +0200
> > > > Daniel Vetter wrote:
> > > >
> > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote:
> > > > > > The HVS block is supposed to fill the pixelvalve FIFOs fast enough
> > > > > > to
> > > > > > meet the requested framerate. The problem is, the HVS and memory bus
> > > > > > bandwidths are limited, and if we don't take these limitations into
> > > > > > account we might end up with HVS underflow errors.
> > > > > >
> > > > > > This patch is trying to model the per-plane HVS and memory bus
> > > > > > bandwidth
> > > > > > consumption and take a decision at atomic_check() time whether the
> > > > > > estimated load will fit in the HVS and membus budget.
> > > > > >
> > > > > > Note that we take an extra margin on the memory bus consumption to
> > > > > > let
> > > > > > the system run smoothly when other blocks are doing heavy use of the
> > > > > > memory bus. Same goes for the HVS limit, except the margin is
> > > > > > smaller in
> > > > > > this case, since the HVS is not used by external components.
> > > > > >
> > > > > > Signed-off-by: Boris Brezillon
> > > > > > ---
> > > > > > This logic has been validated using a simple shell script and
> > > > > > some instrumentation in the VC4 driver:
> > > > > >
> > > > > > - capture underflow errors at the HVS level and expose a debugfs
> > > > > > file
> > > > > > reporting those errors
> > > > > > - add debugfs files to expose when atomic_check fails because of the
> > > > > > HVS or membus load limitation or when it fails for other reasons
> > > > > >
> > > > > > The branch containing those modification is available here [1], and
> > > > > > the
> > > > > > script (which is internally using modetest) is here [2] (please note
> > > > > > that I'm bad at writing shell scripts :-)).
> > > > > >
> > > > > > Note that those modification tend to over-estimate the load, and
> > > > > > thus
> > > > > > reject setups that might have previously worked, so we might want to
> > > > > > adjust the limits to avoid that.
> > > > > >
> > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval
> > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test
> > > > >
> > > > > Any interest in using igt to test this stuff? We have at least a
> > > > > bunch of
> > > > > tests already in there that try all kinds of plane setups. And we use
> > > > > those to hunt for underruns on i915 hw.
> > > > >
> > > > > Wrt underrun reporting: On i915 we just dump them into dmesg at the
> > > > > error
> > > > > level, using DRM_ERROR, plus a tracepoint. See e.g.
> > > > > intel_pch_fifo_underrun_irq_handler(). If there's interest we could
> > > > > perhaps extract this into something common, similar to what was done
> > > > > with
> > > > > crc support already.
> > > >
> > > > Sounds like a good idea. I'll have a look at what's done in the intel
> > > > driver and will check how feasible this is to have a common
> > > > infrastructure to test that in igt.
> > > >
> > > > Thanks for the pointers.
> > > >
> > > >
> > > > > > +static int vc4_load_tracker_atomic_check(struct drm_atomic_state
> > > > > > *state)
> > > > > > +{
> > > > > > + struct drm_plane_state *old_plane_state, *new_plane_state;
> > > > > > + struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> > > > > > + struct vc4_load_tracker_state *load_state;
> > > > > > + struct drm_private_state *priv_state;
> > > > > > + struct drm_plane *plane;
> > > > > > + int ret, i;
> > > > > > +
> > > > >
> > > > > You're missing the modeset locking for vc4->load_tracker. See the
> > > > > kerneldoc for drm_atomic_get_private_obj_state().
> > > >
> > > > Hm, I missed this part of the doc, and I thought we were protected by
> > > > the modeset lock.
> > > >
> > > > > Probably a good time to
> > > > > implement the locking refactoring idea I have and just implement a per
> > > > > private_obj lock, and remove all the ad-hoc locking from all the
> > > > > callers?
> > > >
> > > > Let me see if I get it correctly. You want to add a lock to the
> > > > drm_private_obj struct, right?
> > >
> > > Yup, plus automatically grab that lock in get_private_obj_state, and
> > > then drop the now superfluous locking from all the callers.
> >
> > You mean this?
> > https://patchwork.freedesktop.org/patch/205943/
>
> Yup, that one exactly. No idea why it died in a bikeshed, except maybe we
> should put all the private obj on a list and add some code to
> lock_all_ctx() to also lock those.
Does anyone plan to work on that or should I do it?
___
dri-devel mailing list
dri-devel@l
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Tue, Oct 16, 2018 at 7:39 PM Ville Syrjälä
wrote:
>
> On Tue, Oct 16, 2018 at 06:41:51PM +0200, Daniel Vetter wrote:
> > On Tue, Oct 16, 2018 at 04:28:09PM +0300, Ville Syrjälä wrote:
> > > On Tue, Oct 16, 2018 at 03:12:54PM +0200, Daniel Vetter wrote:
> > > > On Tue, Oct 16, 2018 at 3:10 PM Boris Brezillon
> > > > wrote:
> > > > >
> > > > > Hi Daniel,
> > > > >
> > > > > On Tue, 16 Oct 2018 14:57:43 +0200
> > > > > Daniel Vetter wrote:
> > > > >
> > > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote:
> > > > > > > The HVS block is supposed to fill the pixelvalve FIFOs fast
> > > > > > > enough to
> > > > > > > meet the requested framerate. The problem is, the HVS and memory
> > > > > > > bus
> > > > > > > bandwidths are limited, and if we don't take these limitations
> > > > > > > into
> > > > > > > account we might end up with HVS underflow errors.
> > > > > > >
> > > > > > > This patch is trying to model the per-plane HVS and memory bus
> > > > > > > bandwidth
> > > > > > > consumption and take a decision at atomic_check() time whether the
> > > > > > > estimated load will fit in the HVS and membus budget.
> > > > > > >
> > > > > > > Note that we take an extra margin on the memory bus consumption
> > > > > > > to let
> > > > > > > the system run smoothly when other blocks are doing heavy use of
> > > > > > > the
> > > > > > > memory bus. Same goes for the HVS limit, except the margin is
> > > > > > > smaller in
> > > > > > > this case, since the HVS is not used by external components.
> > > > > > >
> > > > > > > Signed-off-by: Boris Brezillon
> > > > > > > ---
> > > > > > > This logic has been validated using a simple shell script and
> > > > > > > some instrumentation in the VC4 driver:
> > > > > > >
> > > > > > > - capture underflow errors at the HVS level and expose a debugfs
> > > > > > > file
> > > > > > > reporting those errors
> > > > > > > - add debugfs files to expose when atomic_check fails because of
> > > > > > > the
> > > > > > > HVS or membus load limitation or when it fails for other reasons
> > > > > > >
> > > > > > > The branch containing those modification is available here [1],
> > > > > > > and the
> > > > > > > script (which is internally using modetest) is here [2] (please
> > > > > > > note
> > > > > > > that I'm bad at writing shell scripts :-)).
> > > > > > >
> > > > > > > Note that those modification tend to over-estimate the load, and
> > > > > > > thus
> > > > > > > reject setups that might have previously worked, so we might want
> > > > > > > to
> > > > > > > adjust the limits to avoid that.
> > > > > > >
> > > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval
> > > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test
> > > > > >
> > > > > > Any interest in using igt to test this stuff? We have at least a
> > > > > > bunch of
> > > > > > tests already in there that try all kinds of plane setups. And we
> > > > > > use
> > > > > > those to hunt for underruns on i915 hw.
> > > > > >
> > > > > > Wrt underrun reporting: On i915 we just dump them into dmesg at the
> > > > > > error
> > > > > > level, using DRM_ERROR, plus a tracepoint. See e.g.
> > > > > > intel_pch_fifo_underrun_irq_handler(). If there's interest we could
> > > > > > perhaps extract this into something common, similar to what was
> > > > > > done with
> > > > > > crc support already.
> > > > >
> > > > > Sounds like a good idea. I'll have a look at what's done in the intel
> > > > > driver and will check how feasible this is to have a common
> > > > > infrastructure to test that in igt.
> > > > >
> > > > > Thanks for the pointers.
> > > > >
> > > > >
> > > > > > > +static int vc4_load_tracker_atomic_check(struct drm_atomic_state
> > > > > > > *state)
> > > > > > > +{
> > > > > > > + struct drm_plane_state *old_plane_state, *new_plane_state;
> > > > > > > + struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> > > > > > > + struct vc4_load_tracker_state *load_state;
> > > > > > > + struct drm_private_state *priv_state;
> > > > > > > + struct drm_plane *plane;
> > > > > > > + int ret, i;
> > > > > > > +
> > > > > >
> > > > > > You're missing the modeset locking for vc4->load_tracker. See the
> > > > > > kerneldoc for drm_atomic_get_private_obj_state().
> > > > >
> > > > > Hm, I missed this part of the doc, and I thought we were protected by
> > > > > the modeset lock.
> > > > >
> > > > > > Probably a good time to
> > > > > > implement the locking refactoring idea I have and just implement a
> > > > > > per
> > > > > > private_obj lock, and remove all the ad-hoc locking from all the
> > > > > > callers?
> > > > >
> > > > > Let me see if I get it correctly. You want to add a lock to the
> > > > > drm_private_obj struct, right?
> > > >
> > > > Yup, plus automatically grab that lock in get_private_obj_state, and
> > > > then drop the now superfluous locking from all the callers.
> > >
> > > You mean this?
> > > https://patchw
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Tue, Oct 16, 2018 at 06:41:51PM +0200, Daniel Vetter wrote:
> On Tue, Oct 16, 2018 at 04:28:09PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 16, 2018 at 03:12:54PM +0200, Daniel Vetter wrote:
> > > On Tue, Oct 16, 2018 at 3:10 PM Boris Brezillon
> > > wrote:
> > > >
> > > > Hi Daniel,
> > > >
> > > > On Tue, 16 Oct 2018 14:57:43 +0200
> > > > Daniel Vetter wrote:
> > > >
> > > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote:
> > > > > > The HVS block is supposed to fill the pixelvalve FIFOs fast enough
> > > > > > to
> > > > > > meet the requested framerate. The problem is, the HVS and memory bus
> > > > > > bandwidths are limited, and if we don't take these limitations into
> > > > > > account we might end up with HVS underflow errors.
> > > > > >
> > > > > > This patch is trying to model the per-plane HVS and memory bus
> > > > > > bandwidth
> > > > > > consumption and take a decision at atomic_check() time whether the
> > > > > > estimated load will fit in the HVS and membus budget.
> > > > > >
> > > > > > Note that we take an extra margin on the memory bus consumption to
> > > > > > let
> > > > > > the system run smoothly when other blocks are doing heavy use of the
> > > > > > memory bus. Same goes for the HVS limit, except the margin is
> > > > > > smaller in
> > > > > > this case, since the HVS is not used by external components.
> > > > > >
> > > > > > Signed-off-by: Boris Brezillon
> > > > > > ---
> > > > > > This logic has been validated using a simple shell script and
> > > > > > some instrumentation in the VC4 driver:
> > > > > >
> > > > > > - capture underflow errors at the HVS level and expose a debugfs
> > > > > > file
> > > > > > reporting those errors
> > > > > > - add debugfs files to expose when atomic_check fails because of the
> > > > > > HVS or membus load limitation or when it fails for other reasons
> > > > > >
> > > > > > The branch containing those modification is available here [1], and
> > > > > > the
> > > > > > script (which is internally using modetest) is here [2] (please note
> > > > > > that I'm bad at writing shell scripts :-)).
> > > > > >
> > > > > > Note that those modification tend to over-estimate the load, and
> > > > > > thus
> > > > > > reject setups that might have previously worked, so we might want to
> > > > > > adjust the limits to avoid that.
> > > > > >
> > > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval
> > > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test
> > > > >
> > > > > Any interest in using igt to test this stuff? We have at least a
> > > > > bunch of
> > > > > tests already in there that try all kinds of plane setups. And we use
> > > > > those to hunt for underruns on i915 hw.
> > > > >
> > > > > Wrt underrun reporting: On i915 we just dump them into dmesg at the
> > > > > error
> > > > > level, using DRM_ERROR, plus a tracepoint. See e.g.
> > > > > intel_pch_fifo_underrun_irq_handler(). If there's interest we could
> > > > > perhaps extract this into something common, similar to what was done
> > > > > with
> > > > > crc support already.
> > > >
> > > > Sounds like a good idea. I'll have a look at what's done in the intel
> > > > driver and will check how feasible this is to have a common
> > > > infrastructure to test that in igt.
> > > >
> > > > Thanks for the pointers.
> > > >
> > > >
> > > > > > +static int vc4_load_tracker_atomic_check(struct drm_atomic_state
> > > > > > *state)
> > > > > > +{
> > > > > > + struct drm_plane_state *old_plane_state, *new_plane_state;
> > > > > > + struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> > > > > > + struct vc4_load_tracker_state *load_state;
> > > > > > + struct drm_private_state *priv_state;
> > > > > > + struct drm_plane *plane;
> > > > > > + int ret, i;
> > > > > > +
> > > > >
> > > > > You're missing the modeset locking for vc4->load_tracker. See the
> > > > > kerneldoc for drm_atomic_get_private_obj_state().
> > > >
> > > > Hm, I missed this part of the doc, and I thought we were protected by
> > > > the modeset lock.
> > > >
> > > > > Probably a good time to
> > > > > implement the locking refactoring idea I have and just implement a per
> > > > > private_obj lock, and remove all the ad-hoc locking from all the
> > > > > callers?
> > > >
> > > > Let me see if I get it correctly. You want to add a lock to the
> > > > drm_private_obj struct, right?
> > >
> > > Yup, plus automatically grab that lock in get_private_obj_state, and
> > > then drop the now superfluous locking from all the callers.
> >
> > You mean this?
> > https://patchwork.freedesktop.org/patch/205943/
>
> Yup, that one exactly. No idea why it died in a bikeshed, except maybe we
> should put all the private obj on a list and add some code to
> lock_all_ctx() to also lock those.
Yeah, I was also thinking along those lines a while ago when looking
at some function that iterates all the other objects already. Can't
recall what function that
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Tue, Oct 16, 2018 at 04:28:09PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 16, 2018 at 03:12:54PM +0200, Daniel Vetter wrote:
> > On Tue, Oct 16, 2018 at 3:10 PM Boris Brezillon
> > wrote:
> > >
> > > Hi Daniel,
> > >
> > > On Tue, 16 Oct 2018 14:57:43 +0200
> > > Daniel Vetter wrote:
> > >
> > > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote:
> > > > > The HVS block is supposed to fill the pixelvalve FIFOs fast enough to
> > > > > meet the requested framerate. The problem is, the HVS and memory bus
> > > > > bandwidths are limited, and if we don't take these limitations into
> > > > > account we might end up with HVS underflow errors.
> > > > >
> > > > > This patch is trying to model the per-plane HVS and memory bus
> > > > > bandwidth
> > > > > consumption and take a decision at atomic_check() time whether the
> > > > > estimated load will fit in the HVS and membus budget.
> > > > >
> > > > > Note that we take an extra margin on the memory bus consumption to let
> > > > > the system run smoothly when other blocks are doing heavy use of the
> > > > > memory bus. Same goes for the HVS limit, except the margin is smaller
> > > > > in
> > > > > this case, since the HVS is not used by external components.
> > > > >
> > > > > Signed-off-by: Boris Brezillon
> > > > > ---
> > > > > This logic has been validated using a simple shell script and
> > > > > some instrumentation in the VC4 driver:
> > > > >
> > > > > - capture underflow errors at the HVS level and expose a debugfs file
> > > > > reporting those errors
> > > > > - add debugfs files to expose when atomic_check fails because of the
> > > > > HVS or membus load limitation or when it fails for other reasons
> > > > >
> > > > > The branch containing those modification is available here [1], and
> > > > > the
> > > > > script (which is internally using modetest) is here [2] (please note
> > > > > that I'm bad at writing shell scripts :-)).
> > > > >
> > > > > Note that those modification tend to over-estimate the load, and thus
> > > > > reject setups that might have previously worked, so we might want to
> > > > > adjust the limits to avoid that.
> > > > >
> > > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval
> > > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test
> > > >
> > > > Any interest in using igt to test this stuff? We have at least a bunch
> > > > of
> > > > tests already in there that try all kinds of plane setups. And we use
> > > > those to hunt for underruns on i915 hw.
> > > >
> > > > Wrt underrun reporting: On i915 we just dump them into dmesg at the
> > > > error
> > > > level, using DRM_ERROR, plus a tracepoint. See e.g.
> > > > intel_pch_fifo_underrun_irq_handler(). If there's interest we could
> > > > perhaps extract this into something common, similar to what was done
> > > > with
> > > > crc support already.
> > >
> > > Sounds like a good idea. I'll have a look at what's done in the intel
> > > driver and will check how feasible this is to have a common
> > > infrastructure to test that in igt.
> > >
> > > Thanks for the pointers.
> > >
> > >
> > > > > +static int vc4_load_tracker_atomic_check(struct drm_atomic_state
> > > > > *state)
> > > > > +{
> > > > > + struct drm_plane_state *old_plane_state, *new_plane_state;
> > > > > + struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> > > > > + struct vc4_load_tracker_state *load_state;
> > > > > + struct drm_private_state *priv_state;
> > > > > + struct drm_plane *plane;
> > > > > + int ret, i;
> > > > > +
> > > >
> > > > You're missing the modeset locking for vc4->load_tracker. See the
> > > > kerneldoc for drm_atomic_get_private_obj_state().
> > >
> > > Hm, I missed this part of the doc, and I thought we were protected by
> > > the modeset lock.
> > >
> > > > Probably a good time to
> > > > implement the locking refactoring idea I have and just implement a per
> > > > private_obj lock, and remove all the ad-hoc locking from all the
> > > > callers?
> > >
> > > Let me see if I get it correctly. You want to add a lock to the
> > > drm_private_obj struct, right?
> >
> > Yup, plus automatically grab that lock in get_private_obj_state, and
> > then drop the now superfluous locking from all the callers.
>
> You mean this?
> https://patchwork.freedesktop.org/patch/205943/
Yup, that one exactly. No idea why it died in a bikeshed, except maybe we
should put all the private obj on a list and add some code to
lock_all_ctx() to also lock those.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Tue, Oct 16, 2018 at 03:12:54PM +0200, Daniel Vetter wrote:
> On Tue, Oct 16, 2018 at 3:10 PM Boris Brezillon
> wrote:
> >
> > Hi Daniel,
> >
> > On Tue, 16 Oct 2018 14:57:43 +0200
> > Daniel Vetter wrote:
> >
> > > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote:
> > > > The HVS block is supposed to fill the pixelvalve FIFOs fast enough to
> > > > meet the requested framerate. The problem is, the HVS and memory bus
> > > > bandwidths are limited, and if we don't take these limitations into
> > > > account we might end up with HVS underflow errors.
> > > >
> > > > This patch is trying to model the per-plane HVS and memory bus bandwidth
> > > > consumption and take a decision at atomic_check() time whether the
> > > > estimated load will fit in the HVS and membus budget.
> > > >
> > > > Note that we take an extra margin on the memory bus consumption to let
> > > > the system run smoothly when other blocks are doing heavy use of the
> > > > memory bus. Same goes for the HVS limit, except the margin is smaller in
> > > > this case, since the HVS is not used by external components.
> > > >
> > > > Signed-off-by: Boris Brezillon
> > > > ---
> > > > This logic has been validated using a simple shell script and
> > > > some instrumentation in the VC4 driver:
> > > >
> > > > - capture underflow errors at the HVS level and expose a debugfs file
> > > > reporting those errors
> > > > - add debugfs files to expose when atomic_check fails because of the
> > > > HVS or membus load limitation or when it fails for other reasons
> > > >
> > > > The branch containing those modification is available here [1], and the
> > > > script (which is internally using modetest) is here [2] (please note
> > > > that I'm bad at writing shell scripts :-)).
> > > >
> > > > Note that those modification tend to over-estimate the load, and thus
> > > > reject setups that might have previously worked, so we might want to
> > > > adjust the limits to avoid that.
> > > >
> > > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval
> > > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test
> > >
> > > Any interest in using igt to test this stuff? We have at least a bunch of
> > > tests already in there that try all kinds of plane setups. And we use
> > > those to hunt for underruns on i915 hw.
> > >
> > > Wrt underrun reporting: On i915 we just dump them into dmesg at the error
> > > level, using DRM_ERROR, plus a tracepoint. See e.g.
> > > intel_pch_fifo_underrun_irq_handler(). If there's interest we could
> > > perhaps extract this into something common, similar to what was done with
> > > crc support already.
> >
> > Sounds like a good idea. I'll have a look at what's done in the intel
> > driver and will check how feasible this is to have a common
> > infrastructure to test that in igt.
> >
> > Thanks for the pointers.
> >
> >
> > > > +static int vc4_load_tracker_atomic_check(struct drm_atomic_state
> > > > *state)
> > > > +{
> > > > + struct drm_plane_state *old_plane_state, *new_plane_state;
> > > > + struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> > > > + struct vc4_load_tracker_state *load_state;
> > > > + struct drm_private_state *priv_state;
> > > > + struct drm_plane *plane;
> > > > + int ret, i;
> > > > +
> > >
> > > You're missing the modeset locking for vc4->load_tracker. See the
> > > kerneldoc for drm_atomic_get_private_obj_state().
> >
> > Hm, I missed this part of the doc, and I thought we were protected by
> > the modeset lock.
> >
> > > Probably a good time to
> > > implement the locking refactoring idea I have and just implement a per
> > > private_obj lock, and remove all the ad-hoc locking from all the callers?
> >
> > Let me see if I get it correctly. You want to add a lock to the
> > drm_private_obj struct, right?
>
> Yup, plus automatically grab that lock in get_private_obj_state, and
> then drop the now superfluous locking from all the callers.
You mean this?
https://patchwork.freedesktop.org/patch/205943/
--
Ville Syrjälä
Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Tue, Oct 16, 2018 at 3:10 PM Boris Brezillon
wrote:
>
> Hi Daniel,
>
> On Tue, 16 Oct 2018 14:57:43 +0200
> Daniel Vetter wrote:
>
> > On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote:
> > > The HVS block is supposed to fill the pixelvalve FIFOs fast enough to
> > > meet the requested framerate. The problem is, the HVS and memory bus
> > > bandwidths are limited, and if we don't take these limitations into
> > > account we might end up with HVS underflow errors.
> > >
> > > This patch is trying to model the per-plane HVS and memory bus bandwidth
> > > consumption and take a decision at atomic_check() time whether the
> > > estimated load will fit in the HVS and membus budget.
> > >
> > > Note that we take an extra margin on the memory bus consumption to let
> > > the system run smoothly when other blocks are doing heavy use of the
> > > memory bus. Same goes for the HVS limit, except the margin is smaller in
> > > this case, since the HVS is not used by external components.
> > >
> > > Signed-off-by: Boris Brezillon
> > > ---
> > > This logic has been validated using a simple shell script and
> > > some instrumentation in the VC4 driver:
> > >
> > > - capture underflow errors at the HVS level and expose a debugfs file
> > > reporting those errors
> > > - add debugfs files to expose when atomic_check fails because of the
> > > HVS or membus load limitation or when it fails for other reasons
> > >
> > > The branch containing those modification is available here [1], and the
> > > script (which is internally using modetest) is here [2] (please note
> > > that I'm bad at writing shell scripts :-)).
> > >
> > > Note that those modification tend to over-estimate the load, and thus
> > > reject setups that might have previously worked, so we might want to
> > > adjust the limits to avoid that.
> > >
> > > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval
> > > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test
> >
> > Any interest in using igt to test this stuff? We have at least a bunch of
> > tests already in there that try all kinds of plane setups. And we use
> > those to hunt for underruns on i915 hw.
> >
> > Wrt underrun reporting: On i915 we just dump them into dmesg at the error
> > level, using DRM_ERROR, plus a tracepoint. See e.g.
> > intel_pch_fifo_underrun_irq_handler(). If there's interest we could
> > perhaps extract this into something common, similar to what was done with
> > crc support already.
>
> Sounds like a good idea. I'll have a look at what's done in the intel
> driver and will check how feasible this is to have a common
> infrastructure to test that in igt.
>
> Thanks for the pointers.
>
>
> > > +static int vc4_load_tracker_atomic_check(struct drm_atomic_state *state)
> > > +{
> > > + struct drm_plane_state *old_plane_state, *new_plane_state;
> > > + struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> > > + struct vc4_load_tracker_state *load_state;
> > > + struct drm_private_state *priv_state;
> > > + struct drm_plane *plane;
> > > + int ret, i;
> > > +
> >
> > You're missing the modeset locking for vc4->load_tracker. See the
> > kerneldoc for drm_atomic_get_private_obj_state().
>
> Hm, I missed this part of the doc, and I thought we were protected by
> the modeset lock.
>
> > Probably a good time to
> > implement the locking refactoring idea I have and just implement a per
> > private_obj lock, and remove all the ad-hoc locking from all the callers?
>
> Let me see if I get it correctly. You want to add a lock to the
> drm_private_obj struct, right?
Yup, plus automatically grab that lock in get_private_obj_state, and
then drop the now superfluous locking from all the callers.
>
> >
> > Would definitely simplify the code, and avoid "oops no locking" issues
> > like here.
>
> Yep, I agree.
>
> >
> > Cheers, Daniel
> >
> > > + priv_state = drm_atomic_get_private_obj_state(state,
> > > + &vc4->load_tracker);
> > > + if (IS_ERR(priv_state))
> > > + return PTR_ERR(priv_state);
> > > +
> > > + load_state = to_vc4_load_tracker_state(priv_state);
> > > + for_each_oldnew_plane_in_state(state, plane, old_plane_state,
> > > + new_plane_state, i) {
> > > + struct vc4_plane_state *vc4_plane_state;
> > > +
> > > + if (old_plane_state->fb && old_plane_state->crtc) {
> > > + vc4_plane_state = to_vc4_plane_state(old_plane_state);
> > > + load_state->membus_load -=
> > > vc4_plane_state->membus_load;
> > > + load_state->hvs_load -= vc4_plane_state->hvs_load;
> > > + }
> > > +
> > > + if (new_plane_state->fb && new_plane_state->crtc) {
> > > + vc4_plane_state = to_vc4_plane_state(new_plane_state);
> > > + load_state->membus_load +=
> > > vc4_plane_state->membus_load;
> > > + load_state->hvs_load += vc4_pl
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
Hi Daniel,
On Tue, 16 Oct 2018 14:57:43 +0200
Daniel Vetter wrote:
> On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote:
> > The HVS block is supposed to fill the pixelvalve FIFOs fast enough to
> > meet the requested framerate. The problem is, the HVS and memory bus
> > bandwidths are limited, and if we don't take these limitations into
> > account we might end up with HVS underflow errors.
> >
> > This patch is trying to model the per-plane HVS and memory bus bandwidth
> > consumption and take a decision at atomic_check() time whether the
> > estimated load will fit in the HVS and membus budget.
> >
> > Note that we take an extra margin on the memory bus consumption to let
> > the system run smoothly when other blocks are doing heavy use of the
> > memory bus. Same goes for the HVS limit, except the margin is smaller in
> > this case, since the HVS is not used by external components.
> >
> > Signed-off-by: Boris Brezillon
> > ---
> > This logic has been validated using a simple shell script and
> > some instrumentation in the VC4 driver:
> >
> > - capture underflow errors at the HVS level and expose a debugfs file
> > reporting those errors
> > - add debugfs files to expose when atomic_check fails because of the
> > HVS or membus load limitation or when it fails for other reasons
> >
> > The branch containing those modification is available here [1], and the
> > script (which is internally using modetest) is here [2] (please note
> > that I'm bad at writing shell scripts :-)).
> >
> > Note that those modification tend to over-estimate the load, and thus
> > reject setups that might have previously worked, so we might want to
> > adjust the limits to avoid that.
> >
> > [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval
> > [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test
>
> Any interest in using igt to test this stuff? We have at least a bunch of
> tests already in there that try all kinds of plane setups. And we use
> those to hunt for underruns on i915 hw.
>
> Wrt underrun reporting: On i915 we just dump them into dmesg at the error
> level, using DRM_ERROR, plus a tracepoint. See e.g.
> intel_pch_fifo_underrun_irq_handler(). If there's interest we could
> perhaps extract this into something common, similar to what was done with
> crc support already.
Sounds like a good idea. I'll have a look at what's done in the intel
driver and will check how feasible this is to have a common
infrastructure to test that in igt.
Thanks for the pointers.
> > +static int vc4_load_tracker_atomic_check(struct drm_atomic_state *state)
> > +{
> > + struct drm_plane_state *old_plane_state, *new_plane_state;
> > + struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> > + struct vc4_load_tracker_state *load_state;
> > + struct drm_private_state *priv_state;
> > + struct drm_plane *plane;
> > + int ret, i;
> > +
>
> You're missing the modeset locking for vc4->load_tracker. See the
> kerneldoc for drm_atomic_get_private_obj_state().
Hm, I missed this part of the doc, and I thought we were protected by
the modeset lock.
> Probably a good time to
> implement the locking refactoring idea I have and just implement a per
> private_obj lock, and remove all the ad-hoc locking from all the callers?
Let me see if I get it correctly. You want to add a lock to the
drm_private_obj struct, right?
>
> Would definitely simplify the code, and avoid "oops no locking" issues
> like here.
Yep, I agree.
>
> Cheers, Daniel
>
> > + priv_state = drm_atomic_get_private_obj_state(state,
> > + &vc4->load_tracker);
> > + if (IS_ERR(priv_state))
> > + return PTR_ERR(priv_state);
> > +
> > + load_state = to_vc4_load_tracker_state(priv_state);
> > + for_each_oldnew_plane_in_state(state, plane, old_plane_state,
> > + new_plane_state, i) {
> > + struct vc4_plane_state *vc4_plane_state;
> > +
> > + if (old_plane_state->fb && old_plane_state->crtc) {
> > + vc4_plane_state = to_vc4_plane_state(old_plane_state);
> > + load_state->membus_load -= vc4_plane_state->membus_load;
> > + load_state->hvs_load -= vc4_plane_state->hvs_load;
> > + }
> > +
> > + if (new_plane_state->fb && new_plane_state->crtc) {
> > + vc4_plane_state = to_vc4_plane_state(new_plane_state);
> > + load_state->membus_load += vc4_plane_state->membus_load;
> > + load_state->hvs_load += vc4_plane_state->hvs_load;
> > + }
> > + }
> > +
> > + /* The abolsute limit is 2Gbyte/sec, but let's take a margin to let
> > +* the system work when other blocks are accessing the memory.
> > +*/
> > + if (load_state->membus_load > SZ_1G + SZ_512M)
> > + return -ENOSPC;
> > +
> > + /* HVS clock is supposed to run @ 250Mhz, let's take a margin and
> > +* co
Re: [RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
On Tue, Oct 16, 2018 at 11:40:45AM +0200, Boris Brezillon wrote:
> The HVS block is supposed to fill the pixelvalve FIFOs fast enough to
> meet the requested framerate. The problem is, the HVS and memory bus
> bandwidths are limited, and if we don't take these limitations into
> account we might end up with HVS underflow errors.
>
> This patch is trying to model the per-plane HVS and memory bus bandwidth
> consumption and take a decision at atomic_check() time whether the
> estimated load will fit in the HVS and membus budget.
>
> Note that we take an extra margin on the memory bus consumption to let
> the system run smoothly when other blocks are doing heavy use of the
> memory bus. Same goes for the HVS limit, except the margin is smaller in
> this case, since the HVS is not used by external components.
>
> Signed-off-by: Boris Brezillon
> ---
> This logic has been validated using a simple shell script and
> some instrumentation in the VC4 driver:
>
> - capture underflow errors at the HVS level and expose a debugfs file
> reporting those errors
> - add debugfs files to expose when atomic_check fails because of the
> HVS or membus load limitation or when it fails for other reasons
>
> The branch containing those modification is available here [1], and the
> script (which is internally using modetest) is here [2] (please note
> that I'm bad at writing shell scripts :-)).
>
> Note that those modification tend to over-estimate the load, and thus
> reject setups that might have previously worked, so we might want to
> adjust the limits to avoid that.
>
> [1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval
> [2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test
Any interest in using igt to test this stuff? We have at least a bunch of
tests already in there that try all kinds of plane setups. And we use
those to hunt for underruns on i915 hw.
Wrt underrun reporting: On i915 we just dump them into dmesg at the error
level, using DRM_ERROR, plus a tracepoint. See e.g.
intel_pch_fifo_underrun_irq_handler(). If there's interest we could
perhaps extract this into something common, similar to what was done with
crc support already.
> ---
> drivers/gpu/drm/vc4/vc4_drv.h | 11 +
> drivers/gpu/drm/vc4/vc4_kms.c | 104
> +++-
> drivers/gpu/drm/vc4/vc4_plane.c | 60 +++
> 3 files changed, 174 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index bd6ef1f31822..48f6ee5ceda3 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -200,6 +200,7 @@ struct vc4_dev {
>
> struct drm_modeset_lock ctm_state_lock;
> struct drm_private_obj ctm_manager;
> + struct drm_private_obj load_tracker;
> };
>
> static inline struct vc4_dev *
> @@ -369,6 +370,16 @@ struct vc4_plane_state {
>* to enable background color fill.
>*/
> bool needs_bg_fill;
> +
> + /* Load of this plane on the HVS block. The load is expressed in HVS
> + * cycles/sec.
> + */
> + u64 hvs_load;
> +
> + /* Memory bandwidth needed for this plane. This is expressed in
> + * bytes/sec.
> + */
> + u64 membus_load;
> };
>
> static inline struct vc4_plane_state *
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 127468785f74..4c65e6013bd3 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -34,6 +34,18 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct
> drm_private_state *priv)
> return container_of(priv, struct vc4_ctm_state, base);
> }
>
> +struct vc4_load_tracker_state {
> + struct drm_private_state base;
> + u64 hvs_load;
> + u64 membus_load;
> +};
> +
> +static struct vc4_load_tracker_state *
> +to_vc4_load_tracker_state(struct drm_private_state *priv)
> +{
> + return container_of(priv, struct vc4_load_tracker_state, base);
> +}
> +
> static struct vc4_ctm_state *vc4_get_ctm_state(struct drm_atomic_state
> *state,
> struct drm_private_obj *manager)
> {
> @@ -379,6 +391,81 @@ vc4_ctm_atomic_check(struct drm_device *dev, struct
> drm_atomic_state *state)
> return 0;
> }
>
> +static int vc4_load_tracker_atomic_check(struct drm_atomic_state *state)
> +{
> + struct drm_plane_state *old_plane_state, *new_plane_state;
> + struct vc4_dev *vc4 = to_vc4_dev(state->dev);
> + struct vc4_load_tracker_state *load_state;
> + struct drm_private_state *priv_state;
> + struct drm_plane *plane;
> + int ret, i;
> +
You're missing the modeset locking for vc4->load_tracker. See the
kerneldoc for drm_atomic_get_private_obj_state(). Probably a good time to
implement the locking refactoring idea I have and just implement a per
private_obj lock, and remove all the ad-hoc locking from all the callers?
Would definitely simplify the code, and a
[RFC PATCH] drm/vc4: Add a load tracker to prevent HVS underflow errors
The HVS block is supposed to fill the pixelvalve FIFOs fast enough to
meet the requested framerate. The problem is, the HVS and memory bus
bandwidths are limited, and if we don't take these limitations into
account we might end up with HVS underflow errors.
This patch is trying to model the per-plane HVS and memory bus bandwidth
consumption and take a decision at atomic_check() time whether the
estimated load will fit in the HVS and membus budget.
Note that we take an extra margin on the memory bus consumption to let
the system run smoothly when other blocks are doing heavy use of the
memory bus. Same goes for the HVS limit, except the margin is smaller in
this case, since the HVS is not used by external components.
Signed-off-by: Boris Brezillon
---
This logic has been validated using a simple shell script and
some instrumentation in the VC4 driver:
- capture underflow errors at the HVS level and expose a debugfs file
reporting those errors
- add debugfs files to expose when atomic_check fails because of the
HVS or membus load limitation or when it fails for other reasons
The branch containing those modification is available here [1], and the
script (which is internally using modetest) is here [2] (please note
that I'm bad at writing shell scripts :-)).
Note that those modification tend to over-estimate the load, and thus
reject setups that might have previously worked, so we might want to
adjust the limits to avoid that.
[1]https://github.com/bbrezillon/linux/tree/vc4/hvs-bandwidth-eval
[2]https://github.com/bbrezillon/vc4-hvs-bandwidth-test
---
drivers/gpu/drm/vc4/vc4_drv.h | 11 +
drivers/gpu/drm/vc4/vc4_kms.c | 104 +++-
drivers/gpu/drm/vc4/vc4_plane.c | 60 +++
3 files changed, 174 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index bd6ef1f31822..48f6ee5ceda3 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -200,6 +200,7 @@ struct vc4_dev {
struct drm_modeset_lock ctm_state_lock;
struct drm_private_obj ctm_manager;
+ struct drm_private_obj load_tracker;
};
static inline struct vc4_dev *
@@ -369,6 +370,16 @@ struct vc4_plane_state {
* to enable background color fill.
*/
bool needs_bg_fill;
+
+ /* Load of this plane on the HVS block. The load is expressed in HVS
+* cycles/sec.
+*/
+ u64 hvs_load;
+
+ /* Memory bandwidth needed for this plane. This is expressed in
+* bytes/sec.
+*/
+ u64 membus_load;
};
static inline struct vc4_plane_state *
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 127468785f74..4c65e6013bd3 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -34,6 +34,18 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct
drm_private_state *priv)
return container_of(priv, struct vc4_ctm_state, base);
}
+struct vc4_load_tracker_state {
+ struct drm_private_state base;
+ u64 hvs_load;
+ u64 membus_load;
+};
+
+static struct vc4_load_tracker_state *
+to_vc4_load_tracker_state(struct drm_private_state *priv)
+{
+ return container_of(priv, struct vc4_load_tracker_state, base);
+}
+
static struct vc4_ctm_state *vc4_get_ctm_state(struct drm_atomic_state *state,
struct drm_private_obj *manager)
{
@@ -379,6 +391,81 @@ vc4_ctm_atomic_check(struct drm_device *dev, struct
drm_atomic_state *state)
return 0;
}
+static int vc4_load_tracker_atomic_check(struct drm_atomic_state *state)
+{
+ struct drm_plane_state *old_plane_state, *new_plane_state;
+ struct vc4_dev *vc4 = to_vc4_dev(state->dev);
+ struct vc4_load_tracker_state *load_state;
+ struct drm_private_state *priv_state;
+ struct drm_plane *plane;
+ int ret, i;
+
+ priv_state = drm_atomic_get_private_obj_state(state,
+ &vc4->load_tracker);
+ if (IS_ERR(priv_state))
+ return PTR_ERR(priv_state);
+
+ load_state = to_vc4_load_tracker_state(priv_state);
+ for_each_oldnew_plane_in_state(state, plane, old_plane_state,
+ new_plane_state, i) {
+ struct vc4_plane_state *vc4_plane_state;
+
+ if (old_plane_state->fb && old_plane_state->crtc) {
+ vc4_plane_state = to_vc4_plane_state(old_plane_state);
+ load_state->membus_load -= vc4_plane_state->membus_load;
+ load_state->hvs_load -= vc4_plane_state->hvs_load;
+ }
+
+ if (new_plane_state->fb && new_plane_state->crtc) {
+ vc4_plane_state = to_vc4_plane_state(new_plane_state);
+ load_state->membus_load += vc4_plane_state->membus_load;
+
