| Message ID | 1465827229-1704-4-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 13, 2016 at 04:13:48PM +0200, Daniel Vetter wrote: > Currently it's part of prepare_fb, still in the first phase of > atomic_commit which might fail. Which means that we need to have some > heuristics in cleanup_fb to figure out whether things failed, or > whether we just clean up the old fbs. > > That's fragile, and worse, once we start pipelining commits gets > confused: While the last commit is still getting cleanup up we already > hammer in the new one, and fb_bits aren't refcounted, resulting in > lost bits and WARN_ON galore. We could instead try to make cleanup_fb > more clever, but a simpler fix is to postpone the fb_bits tracking > past the point of no return, where we commit all the software state. > > That also makes conceptually more sense, since fb_bits must be updated > synchronously from the ioctl (they track usage from userspace pov, not > from the hw pov), right before we're fully committed to the updated. > > This fixes WARNING splats from track_fb with page_flip implemented > through atomic_commit. > > Testcase: igt/kms_flip/flip-vs-rmfb > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 41 ++++++++++++++++++++++-------------- > 1 file changed, 25 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a16a307dbb76..fd46db4882e5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13863,6 +13863,25 @@ static void intel_atomic_commit_work(struct work_struct *work) > intel_atomic_commit_tail(state); > } > > +static void intel_atomic_track_fbs(struct drm_atomic_state *state) > +{ > + struct drm_plane_state *old_plane_state; > + struct drm_plane *plane; > + struct drm_i915_gem_object *obj, *old_obj; > + struct intel_plane *intel_plane; > + int i; > + > + mutex_lock(&state->dev->struct_mutex); > + for_each_plane_in_state(state, plane, old_plane_state, i) { > + obj = intel_fb_obj(plane->state->fb); > + old_obj = intel_fb_obj(old_plane_state->fb); > + intel_plane = to_intel_plane(plane); > + > + i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit); > + } > + mutex_unlock(&state->dev->struct_mutex); > +} Seems like an opportune time to pull in https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=d16c3d8f3dd395b8d13a3691efb356a23e0b702b https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=4a5c85282ba3c15a64dc0f600969234f30ebe820 and fwiw i915_gem_track_fb would be better off inside intel_display.c -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a16a307dbb76..fd46db4882e5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13863,6 +13863,25 @@ static void intel_atomic_commit_work(struct work_struct *work) intel_atomic_commit_tail(state); } +static void intel_atomic_track_fbs(struct drm_atomic_state *state) +{ + struct drm_plane_state *old_plane_state; + struct drm_plane *plane; + struct drm_i915_gem_object *obj, *old_obj; + struct intel_plane *intel_plane; + int i; + + mutex_lock(&state->dev->struct_mutex); + for_each_plane_in_state(state, plane, old_plane_state, i) { + obj = intel_fb_obj(plane->state->fb); + old_obj = intel_fb_obj(old_plane_state->fb); + intel_plane = to_intel_plane(plane); + + i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit); + } + mutex_unlock(&state->dev->struct_mutex); +} + /** * intel_atomic_commit - commit validated state object * @dev: DRM device @@ -13903,6 +13922,7 @@ static int intel_atomic_commit(struct drm_device *dev, dev_priv->wm.distrust_bios_wm = false; dev_priv->wm.skl_results = intel_state->wm_results; intel_shared_dpll_commit(state); + intel_atomic_track_fbs(state); if (nonblock) queue_work(system_unbound_wq, &state->commit_work); @@ -13982,7 +14002,6 @@ intel_prepare_plane_fb(struct drm_plane *plane, { struct drm_device *dev = plane->dev; struct drm_framebuffer *fb = new_state->fb; - struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_i915_gem_object *obj = intel_fb_obj(fb); struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb); int ret = 0; @@ -14039,16 +14058,12 @@ intel_prepare_plane_fb(struct drm_plane *plane, ret = intel_pin_and_fence_fb_obj(fb, new_state->rotation); } - if (ret == 0) { - if (obj) { - struct intel_plane_state *plane_state = - to_intel_plane_state(new_state); - - i915_gem_request_assign(&plane_state->wait_req, - obj->last_write_req); - } + if (ret == 0 && obj) { + struct intel_plane_state *plane_state = + to_intel_plane_state(new_state); - i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit); + i915_gem_request_assign(&plane_state->wait_req, + obj->last_write_req); } return ret; @@ -14068,7 +14083,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane, const struct drm_plane_state *old_state) { struct drm_device *dev = plane->dev; - struct intel_plane *intel_plane = to_intel_plane(plane); struct intel_plane_state *old_intel_state; struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb); struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb); @@ -14082,11 +14096,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane, !INTEL_INFO(dev)->cursor_needs_physical)) intel_unpin_fb_obj(old_state->fb, old_state->rotation); - /* prepare_fb aborted? */ - if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) || - (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit))) - i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit); - i915_gem_request_assign(&old_intel_state->wait_req, NULL); }
Currently it's part of prepare_fb, still in the first phase of atomic_commit which might fail. Which means that we need to have some heuristics in cleanup_fb to figure out whether things failed, or whether we just clean up the old fbs. That's fragile, and worse, once we start pipelining commits gets confused: While the last commit is still getting cleanup up we already hammer in the new one, and fb_bits aren't refcounted, resulting in lost bits and WARN_ON galore. We could instead try to make cleanup_fb more clever, but a simpler fix is to postpone the fb_bits tracking past the point of no return, where we commit all the software state. That also makes conceptually more sense, since fb_bits must be updated synchronously from the ioctl (they track usage from userspace pov, not from the hw pov), right before we're fully committed to the updated. This fixes WARNING splats from track_fb with page_flip implemented through atomic_commit. Testcase: igt/kms_flip/flip-vs-rmfb Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 41 ++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 16 deletions(-)