| Message ID | 1478883461-20201-3-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > 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. -Chris
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@linux.intel.com> > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > 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@linux.intel.com> 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.
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)); - intel_crtc_state->enable_fbc = true; - goto out; - } + intel_crtc_state->enable_fbc = true; + break; } out:
Much simpler. Thanks to Ville for pointing this. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_fbc.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)