On Fri, Nov 11, 2016 at 05:46:16PM +0000, Chris Wilson wrote: > On Fri, Nov 11, 2016 at 02:57:36PM -0200, Paulo Zanoni wrote: > > Much simpler. Thanks to Ville for pointing this. > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Reported-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/intel_fbc.c | 17 +++++++---------- > > 1 file changed, 7 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > > index ded77bd..b53b884 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -1071,7 +1071,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv, > > struct drm_plane *plane; > > struct drm_plane_state *plane_state; > > bool fbc_crtc_present = false; > > - int i, j; > > + int i; > > > > mutex_lock(&fbc->lock); > > > > @@ -1092,6 +1092,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv, > > for_each_plane_in_state(state, plane, plane_state, i) { > > struct intel_plane_state *intel_plane_state = > > to_intel_plane_state(plane_state); > > + struct intel_crtc_state *intel_crtc_state; > > > > if (!intel_plane_state->base.visible) > > continue; > > @@ -1099,16 +1100,12 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv, > > if (!intel_fbc_can_choose(to_intel_crtc(plane_state->crtc))) > > continue; > > > > - for_each_crtc_in_state(state, crtc, crtc_state, j) { > > - struct intel_crtc_state *intel_crtc_state = > > - to_intel_crtc_state(crtc_state); > > - > > - if (plane_state->crtc != crtc) > > - continue; > > + intel_crtc_state = to_intel_crtc_state( > > + drm_atomic_get_existing_crtc_state(state, > > + plane_state->crtc)); > > My knowledge of atomic api is not good enough to say whether this can > return NULL at this point. If the plane is part of the state, then plane_state->crtc will be part of the state as well. Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> Just to reaffirm I wanted to point out what drm_atomic_set_crtc_for_plane() does, but looks like that one was written with the assumption that it could fail, which is not correct since drm_atomic_get_plane_state() will always add the current crtc to the state, and drm_atomic_set_crtc_for_plane() will always add the new crtc to the state. drm_atomic_set_crtc_for_connector() is written in the way I was expecting, so I think I'll fire off a patch for drm_atomic_set_crtc_for_plane() to follow the same pattern. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx