| Message ID | 1476707838-25253-17-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 17, 2016 at 02:37:15PM +0200, Maarten Lankhorst wrote: > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_fbc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index faa67624e1ed..0028335fc1bb 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -1060,7 +1060,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv, > > mutex_lock(&fbc->lock); > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > if (fbc->crtc == to_intel_crtc(crtc)) { > fbc_crtc_present = true; > break; > @@ -1074,14 +1074,14 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv, > * plane. We could go for fancier schemes such as checking the plane > * size, but this would just affect the few platforms that don't tie FBC > * to pipe or plane A. */ > - for_each_plane_in_state(state, plane, plane_state, i) { > + for_each_new_plane_in_state(state, plane, plane_state, i) { > struct intel_plane_state *intel_plane_state = > to_intel_plane_state(plane_state); > > if (!intel_plane_state->base.visible) > continue; Unrelated but this thing looks somewhat bogus. FBC is tied to the primary plane only, so why do we care about the visibility of the other planes? Adding Paulo to Cc... > > - for_each_crtc_in_state(state, crtc, crtc_state, j) { > + for_each_new_crtc_in_state(state, crtc, crtc_state, j) { Also, can't this inner loop be replaced with a simple crtc = plane_state->crtc ? > struct intel_crtc_state *intel_crtc_state = > to_intel_crtc_state(crtc_state); > > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Qui, 2016-11-03 às 18:45 +0200, Ville Syrjälä escreveu: > On Mon, Oct 17, 2016 at 02:37:15PM +0200, Maarten Lankhorst wrote: > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com > > > > > --- > > drivers/gpu/drm/i915/intel_fbc.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > b/drivers/gpu/drm/i915/intel_fbc.c > > index faa67624e1ed..0028335fc1bb 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -1060,7 +1060,7 @@ void intel_fbc_choose_crtc(struct > > drm_i915_private *dev_priv, > > > > mutex_lock(&fbc->lock); > > > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > > if (fbc->crtc == to_intel_crtc(crtc)) { > > fbc_crtc_present = true; > > break; > > @@ -1074,14 +1074,14 @@ void intel_fbc_choose_crtc(struct > > drm_i915_private *dev_priv, > > * plane. We could go for fancier schemes such as checking > > the plane > > * size, but this would just affect the few platforms that > > don't tie FBC > > * to pipe or plane A. */ > > - for_each_plane_in_state(state, plane, plane_state, i) { > > + for_each_new_plane_in_state(state, plane, plane_state, i) > > { > > struct intel_plane_state *intel_plane_state = > > to_intel_plane_state(plane_state); > > > > if (!intel_plane_state->base.visible) > > continue; > > Unrelated but this thing looks somewhat bogus. FBC is tied to the > primary plane only, so why do we care about the visibility of the > other > planes? Adding Paulo to Cc... Looks like you've found a bug... Thanks! We should really be iterating over primary planes only. Adding to the TODO list. > > > > > > > - for_each_crtc_in_state(state, crtc, crtc_state, j) > > { > > + for_each_new_crtc_in_state(state, crtc, > > crtc_state, j) { > > Also, can't this inner loop be replaced with a simple > crtc = plane_state->crtc ? Is there a way to get the proposed crtc_state without the loop? > > > > > struct intel_crtc_state *intel_crtc_state > > = > > to_intel_crtc_state(crtc_state); > > > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Nov 03, 2016 at 03:45:43PM -0200, Paulo Zanoni wrote: > Em Qui, 2016-11-03 às 18:45 +0200, Ville Syrjälä escreveu: > > On Mon, Oct 17, 2016 at 02:37:15PM +0200, Maarten Lankhorst wrote: > > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com > > > > > > > --- > > > drivers/gpu/drm/i915/intel_fbc.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > index faa67624e1ed..0028335fc1bb 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > @@ -1060,7 +1060,7 @@ void intel_fbc_choose_crtc(struct > > > drm_i915_private *dev_priv, > > > > > > mutex_lock(&fbc->lock); > > > > > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > > > if (fbc->crtc == to_intel_crtc(crtc)) { > > > fbc_crtc_present = true; > > > break; > > > @@ -1074,14 +1074,14 @@ void intel_fbc_choose_crtc(struct > > > drm_i915_private *dev_priv, > > > * plane. We could go for fancier schemes such as checking > > > the plane > > > * size, but this would just affect the few platforms that > > > don't tie FBC > > > * to pipe or plane A. */ > > > - for_each_plane_in_state(state, plane, plane_state, i) { > > > + for_each_new_plane_in_state(state, plane, plane_state, i) > > > { > > > struct intel_plane_state *intel_plane_state = > > > to_intel_plane_state(plane_state); > > > > > > if (!intel_plane_state->base.visible) > > > continue; > > > > Unrelated but this thing looks somewhat bogus. FBC is tied to the > > primary plane only, so why do we care about the visibility of the > > other > > planes? Adding Paulo to Cc... > > Looks like you've found a bug... Thanks! We should really be iterating > over primary planes only. Adding to the TODO list. > > > > > > > > > > > > - for_each_crtc_in_state(state, crtc, crtc_state, j) > > > { > > > + for_each_new_crtc_in_state(state, crtc, > > > crtc_state, j) { > > > > Also, can't this inner loop be replaced with a simple > > crtc = plane_state->crtc ? > > Is there a way to get the proposed crtc_state without the loop? ...get_existing_crtc_state(plane_state->crtc); since the plane is part of the state the crtc should be too, I think.
On Thu, Nov 03, 2016 at 07:55:20PM +0200, Ville Syrjälä wrote: > On Thu, Nov 03, 2016 at 03:45:43PM -0200, Paulo Zanoni wrote: > > Em Qui, 2016-11-03 às 18:45 +0200, Ville Syrjälä escreveu: > > > On Mon, Oct 17, 2016 at 02:37:15PM +0200, Maarten Lankhorst wrote: > > > > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com > > > > > > > > > --- > > > > drivers/gpu/drm/i915/intel_fbc.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > > index faa67624e1ed..0028335fc1bb 100644 > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > > @@ -1060,7 +1060,7 @@ void intel_fbc_choose_crtc(struct > > > > drm_i915_private *dev_priv, > > > > > > > > mutex_lock(&fbc->lock); > > > > > > > > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > > > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > > > > if (fbc->crtc == to_intel_crtc(crtc)) { > > > > fbc_crtc_present = true; > > > > break; > > > > @@ -1074,14 +1074,14 @@ void intel_fbc_choose_crtc(struct > > > > drm_i915_private *dev_priv, > > > > * plane. We could go for fancier schemes such as checking > > > > the plane > > > > * size, but this would just affect the few platforms that > > > > don't tie FBC > > > > * to pipe or plane A. */ > > > > - for_each_plane_in_state(state, plane, plane_state, i) { > > > > + for_each_new_plane_in_state(state, plane, plane_state, i) > > > > { > > > > struct intel_plane_state *intel_plane_state = > > > > to_intel_plane_state(plane_state); > > > > > > > > if (!intel_plane_state->base.visible) > > > > continue; > > > > > > Unrelated but this thing looks somewhat bogus. FBC is tied to the > > > primary plane only, so why do we care about the visibility of the > > > other > > > planes? Adding Paulo to Cc... > > > > Looks like you've found a bug... Thanks! We should really be iterating > > over primary planes only. Adding to the TODO list. > > > > > > > > > > > > > > > > > - for_each_crtc_in_state(state, crtc, crtc_state, j) > > > > { > > > > + for_each_new_crtc_in_state(state, crtc, > > > > crtc_state, j) { > > > > > > Also, can't this inner loop be replaced with a simple > > > crtc = plane_state->crtc ? > > > > Is there a way to get the proposed crtc_state without the loop? > > ...get_existing_crtc_state(plane_state->crtc); > > since the plane is part of the state the crtc should be too, I think. Well, I guess Maarten wants to change it to get_new_state() soon enough.
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index faa67624e1ed..0028335fc1bb 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -1060,7 +1060,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv, mutex_lock(&fbc->lock); - for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { if (fbc->crtc == to_intel_crtc(crtc)) { fbc_crtc_present = true; break; @@ -1074,14 +1074,14 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv, * plane. We could go for fancier schemes such as checking the plane * size, but this would just affect the few platforms that don't tie FBC * to pipe or plane A. */ - for_each_plane_in_state(state, plane, plane_state, i) { + for_each_new_plane_in_state(state, plane, plane_state, i) { struct intel_plane_state *intel_plane_state = to_intel_plane_state(plane_state); if (!intel_plane_state->base.visible) continue; - for_each_crtc_in_state(state, crtc, crtc_state, j) { + for_each_new_crtc_in_state(state, crtc, crtc_state, j) { struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_fbc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)