| Message ID | 20180919135644.14182-3-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gen11: Implement planar format support. | expand |
On Wed, Sep 19, 2018 at 03:56:31PM +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. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Fixes: b2b55502d683c ("drm/i915: Pass proper old/new states to intel_plane_atomic_check_with_state()") I think? So basically we were clobbering the current cstate's active_planes field (and anything else in cstate that intel_plane_atomic_check_with_state() might touch in the future) if the atomic transaction failed. > --- > drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2ac381eb8103..7a7ff1d76031 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13517,14 +13517,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; > @@ -13554,6 +13556,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; > @@ -13565,9 +13573,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; > @@ -13588,11 +13595,11 @@ intel_legacy_cursor_update(struct drm_plane *plane, > > /* Swap plane state */ > plane->state = new_plane_state; > + crtc_state->active_planes = new_crtc_state->active_planes; At the moment this is the only crtc_state field that our plane check might update for a cursor, but is there a reason why we don't just swap in the new state and destroy the old one farther down? If our driver ever did start updating other fields, it would be really easy to overlook the need to copy those new fields over here. I realize a lot of the likely candidates for fields that plane check might want to update (e.g., watermark stuff) would also disqualify us from taking this legacy cursor fastpath in the first place, but I'm a bit nervous about assuming that we'll never be updating any other fields at all. Matt > > 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)); > @@ -13604,6 +13611,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 20-09-18 om 02:12 schreef Matt Roper: > On Wed, Sep 19, 2018 at 03:56:31PM +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. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Fixes: b2b55502d683c ("drm/i915: Pass proper old/new states to > intel_plane_atomic_check_with_state()") > > I think? So basically we were clobbering the current cstate's > active_planes field (and anything else in cstate that > intel_plane_atomic_check_with_state() might touch in the future) if the > atomic transaction failed. This is in theory a fix, but up until now we don't depend on it. It only starts mattering further down where we start to updating/disabling planes based on this field. This is why I didn't add -fixes. >> --- >> drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++-------- >> 1 file changed, 17 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 2ac381eb8103..7a7ff1d76031 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -13517,14 +13517,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; >> @@ -13554,6 +13556,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; >> @@ -13565,9 +13573,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; >> @@ -13588,11 +13595,11 @@ intel_legacy_cursor_update(struct drm_plane *plane, >> >> /* Swap plane state */ >> plane->state = new_plane_state; >> + crtc_state->active_planes = new_crtc_state->active_planes; > At the moment this is the only crtc_state field that our plane check > might update for a cursor, but is there a reason why we don't just swap > in the new state and destroy the old one farther down? If our driver > ever did start updating other fields, it would be really easy to > overlook the need to copy those new fields over here. I realize a lot > of the likely candidates for fields that plane check might want to > update (e.g., watermark stuff) would also disqualify us from taking this > legacy cursor fastpath in the first place, but I'm a bit nervous about > assuming that we'll never be updating any other fields at all. I wish! This was what I first tried. Then realized it was a very very bad idea. We wait for plane_state->hw_done to be set, but an atomic update may be in progress. In which case our old_crtc_state is its new_crtc_state. If we start freeing that.. really bad things happen. (TM) I'll add a comment here to explain why we only update crtc_state. In theory it could be a problem that we update crtc_state->active_planes life, but the place that will depend on it checks if the plane is part of the atomic commit, and we ruled out that's the case. So even if we change it, either value will cause the same behavior when running the atomic plane update part. We've already ruled out modesets, so we won't race with disabling all planes. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ac381eb8103..7a7ff1d76031 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13517,14 +13517,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; @@ -13554,6 +13556,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; @@ -13565,9 +13573,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; @@ -13588,11 +13595,11 @@ intel_legacy_cursor_update(struct drm_plane *plane, /* Swap plane state */ plane->state = new_plane_state; + 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)); @@ -13604,6 +13611,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. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)