| Message ID | 20180920102711.4184-3-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Preparations for adding gen11 planar formats. | expand |
On Thu, Sep 20, 2018 at 12:27:05PM +0200, Maarten Lankhorst wrote: > While we may not update new_crtc_state, we may clear active_planes > if the new cursor update state will disable the cursor, but we fail > after. If this is immediately followed by a modeset disable, we may > soon not disable the planes correctly when we start depending on > active_planes. > > Changes since v1: > - Clarify why we cannot swap crtc_state. (Matt) > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++------- > 1 file changed, 28 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 58c188482c78..078cdcca88e1 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13515,14 +13515,16 @@ intel_legacy_cursor_update(struct drm_plane *plane, > struct drm_plane_state *old_plane_state, *new_plane_state; > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_framebuffer *old_fb; > - struct drm_crtc_state *crtc_state = crtc->state; > + struct intel_crtc_state *crtc_state = > + to_intel_crtc_state(crtc->state); > + struct intel_crtc_state *new_crtc_state; > > /* > * When crtc is inactive or there is a modeset pending, > * wait for it to complete in the slowpath > */ > - if (!crtc_state->active || needs_modeset(crtc_state) || > - to_intel_crtc_state(crtc_state)->update_pipe) > + if (!crtc_state->base.active || needs_modeset(&crtc_state->base) || > + crtc_state->update_pipe) > goto slow; > > old_plane_state = plane->state; > @@ -13552,6 +13554,12 @@ intel_legacy_cursor_update(struct drm_plane *plane, > if (!new_plane_state) > return -ENOMEM; > > + new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc)); > + if (!new_crtc_state) { > + ret = -ENOMEM; > + goto out_free; > + } > + > drm_atomic_set_fb_for_plane(new_plane_state, fb); > > new_plane_state->src_x = src_x; > @@ -13563,9 +13571,8 @@ intel_legacy_cursor_update(struct drm_plane *plane, > new_plane_state->crtc_w = crtc_w; > new_plane_state->crtc_h = crtc_h; > > - ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state), > - to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */ > - to_intel_plane_state(plane->state), > + ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state, > + to_intel_plane_state(old_plane_state), > to_intel_plane_state(new_plane_state)); > if (ret) > goto out_free; > @@ -13587,10 +13594,21 @@ intel_legacy_cursor_update(struct drm_plane *plane, > /* Swap plane state */ > plane->state = new_plane_state; > > + /* > + * We cannot swap crtc_state as it may be in use by an atomic commit or > + * page flip that's running simultaneously. If we swap crtc_state and > + * destroy the old state, we will cause a use-after-free there. Just to confirm, the commit running simultaneously would have to have already dropped locks (specifically the crtc lock) and returned to userspace for us to have this problem, right? So it's either a non-blocking commit in the process of doing its programming via workqueue, or a blocking commit that's done everything except intel_atomic_cleanup_work? Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > + * > + * Only update active_planes, which is needed for our internal > + * bookkeeping. Either value will do the right thing when updating > + * planes atomically. If the cursor was part of the atomic update then > + * we would have taken the slowpath. > + */ > + crtc_state->active_planes = new_crtc_state->active_planes; > + > if (plane->state->visible) { > trace_intel_update_plane(plane, to_intel_crtc(crtc)); > - intel_plane->update_plane(intel_plane, > - to_intel_crtc_state(crtc->state), > + intel_plane->update_plane(intel_plane, crtc_state, > to_intel_plane_state(plane->state)); > } else { > trace_intel_disable_plane(plane, to_intel_crtc(crtc)); > @@ -13602,6 +13620,8 @@ intel_legacy_cursor_update(struct drm_plane *plane, > out_unlock: > mutex_unlock(&dev_priv->drm.struct_mutex); > out_free: > + if (new_crtc_state) > + intel_crtc_destroy_state(crtc, &new_crtc_state->base); > if (ret) > intel_plane_destroy_state(plane, new_plane_state); > else > -- > 2.18.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 21-09-18 om 01:18 schreef Matt Roper: > On Thu, Sep 20, 2018 at 12:27:05PM +0200, Maarten Lankhorst wrote: >> While we may not update new_crtc_state, we may clear active_planes >> if the new cursor update state will disable the cursor, but we fail >> after. If this is immediately followed by a modeset disable, we may >> soon not disable the planes correctly when we start depending on >> active_planes. >> >> Changes since v1: >> - Clarify why we cannot swap crtc_state. (Matt) >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++------- >> 1 file changed, 28 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 58c188482c78..078cdcca88e1 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -13515,14 +13515,16 @@ intel_legacy_cursor_update(struct drm_plane *plane, >> struct drm_plane_state *old_plane_state, *new_plane_state; >> struct intel_plane *intel_plane = to_intel_plane(plane); >> struct drm_framebuffer *old_fb; >> - struct drm_crtc_state *crtc_state = crtc->state; >> + struct intel_crtc_state *crtc_state = >> + to_intel_crtc_state(crtc->state); >> + struct intel_crtc_state *new_crtc_state; >> >> /* >> * When crtc is inactive or there is a modeset pending, >> * wait for it to complete in the slowpath >> */ >> - if (!crtc_state->active || needs_modeset(crtc_state) || >> - to_intel_crtc_state(crtc_state)->update_pipe) >> + if (!crtc_state->base.active || needs_modeset(&crtc_state->base) || >> + crtc_state->update_pipe) >> goto slow; >> >> old_plane_state = plane->state; >> @@ -13552,6 +13554,12 @@ intel_legacy_cursor_update(struct drm_plane *plane, >> if (!new_plane_state) >> return -ENOMEM; >> >> + new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc)); >> + if (!new_crtc_state) { >> + ret = -ENOMEM; >> + goto out_free; >> + } >> + >> drm_atomic_set_fb_for_plane(new_plane_state, fb); >> >> new_plane_state->src_x = src_x; >> @@ -13563,9 +13571,8 @@ intel_legacy_cursor_update(struct drm_plane *plane, >> new_plane_state->crtc_w = crtc_w; >> new_plane_state->crtc_h = crtc_h; >> >> - ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state), >> - to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */ >> - to_intel_plane_state(plane->state), >> + ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state, >> + to_intel_plane_state(old_plane_state), >> to_intel_plane_state(new_plane_state)); >> if (ret) >> goto out_free; >> @@ -13587,10 +13594,21 @@ intel_legacy_cursor_update(struct drm_plane *plane, >> /* Swap plane state */ >> plane->state = new_plane_state; >> >> + /* >> + * We cannot swap crtc_state as it may be in use by an atomic commit or >> + * page flip that's running simultaneously. If we swap crtc_state and >> + * destroy the old state, we will cause a use-after-free there. > Just to confirm, the commit running simultaneously would have to have > already dropped locks (specifically the crtc lock) and returned to > userspace for us to have this problem, right? So it's either a > non-blocking commit in the process of doing its programming via > workqueue, or a blocking commit that's done everything except > intel_atomic_cleanup_work? > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> Anything before drm_atomic_helper_commit_hw_done(). Cleanup work is done after, and isn't allowed to look at new state any more. :)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 58c188482c78..078cdcca88e1 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13515,14 +13515,16 @@ intel_legacy_cursor_update(struct drm_plane *plane, struct drm_plane_state *old_plane_state, *new_plane_state; struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_framebuffer *old_fb; - struct drm_crtc_state *crtc_state = crtc->state; + struct intel_crtc_state *crtc_state = + to_intel_crtc_state(crtc->state); + struct intel_crtc_state *new_crtc_state; /* * When crtc is inactive or there is a modeset pending, * wait for it to complete in the slowpath */ - if (!crtc_state->active || needs_modeset(crtc_state) || - to_intel_crtc_state(crtc_state)->update_pipe) + if (!crtc_state->base.active || needs_modeset(&crtc_state->base) || + crtc_state->update_pipe) goto slow; old_plane_state = plane->state; @@ -13552,6 +13554,12 @@ intel_legacy_cursor_update(struct drm_plane *plane, if (!new_plane_state) return -ENOMEM; + new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc)); + if (!new_crtc_state) { + ret = -ENOMEM; + goto out_free; + } + drm_atomic_set_fb_for_plane(new_plane_state, fb); new_plane_state->src_x = src_x; @@ -13563,9 +13571,8 @@ intel_legacy_cursor_update(struct drm_plane *plane, new_plane_state->crtc_w = crtc_w; new_plane_state->crtc_h = crtc_h; - ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state), - to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */ - to_intel_plane_state(plane->state), + ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state, + to_intel_plane_state(old_plane_state), to_intel_plane_state(new_plane_state)); if (ret) goto out_free; @@ -13587,10 +13594,21 @@ intel_legacy_cursor_update(struct drm_plane *plane, /* Swap plane state */ plane->state = new_plane_state; + /* + * We cannot swap crtc_state as it may be in use by an atomic commit or + * page flip that's running simultaneously. If we swap crtc_state and + * destroy the old state, we will cause a use-after-free there. + * + * Only update active_planes, which is needed for our internal + * bookkeeping. Either value will do the right thing when updating + * planes atomically. If the cursor was part of the atomic update then + * we would have taken the slowpath. + */ + crtc_state->active_planes = new_crtc_state->active_planes; + if (plane->state->visible) { trace_intel_update_plane(plane, to_intel_crtc(crtc)); - intel_plane->update_plane(intel_plane, - to_intel_crtc_state(crtc->state), + intel_plane->update_plane(intel_plane, crtc_state, to_intel_plane_state(plane->state)); } else { trace_intel_disable_plane(plane, to_intel_crtc(crtc)); @@ -13602,6 +13620,8 @@ intel_legacy_cursor_update(struct drm_plane *plane, out_unlock: mutex_unlock(&dev_priv->drm.struct_mutex); out_free: + if (new_crtc_state) + intel_crtc_destroy_state(crtc, &new_crtc_state->base); if (ret) intel_plane_destroy_state(plane, new_plane_state); else
While we may not update new_crtc_state, we may clear active_planes if the new cursor update state will disable the cursor, but we fail after. If this is immediately followed by a modeset disable, we may soon not disable the planes correctly when we start depending on active_planes. Changes since v1: - Clarify why we cannot swap crtc_state. (Matt) Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++------- 1 file changed, 28 insertions(+), 8 deletions(-)