Hi Daniel, On Tue, 16 Oct 2018 14:57:43 +0200 Daniel Vetter <daniel@xxxxxxxx> 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 <boris.brezillon@xxxxxxxxxxx> > > --- > > 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 > > + * consider the maximum number of cycles is 240M. > > + */ > > + if (load_state->hvs_load > 240000000ULL) > > + return -ENOSPC; > > EINVAL is for atomic_check failures. ENOSPC isn't one of the permitted > errno codes, see the kernel-doc for &drm_mode_config_funcs.atomic_check. > atomic_commit has a different set of permissible errno codes. Okay, will change ENOSPC into EINVAL. > > We should probably enforce this in drm core ... Enough on my plate for now, I'll let someone else take care of that if you don't mind :-P. Thanks for the review. Boris _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
