Re: [Intel-gfx] [PATCH 16/19] drm/i915: Use new atomic iterator macros in fbc
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 > > > >
> > > > ---
> > > > 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.
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 16/19] drm/i915: Use new atomic iterator macros in fbc
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 > > >
> > > ---
> > > 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.
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 16/19] drm/i915: Use new atomic iterator macros in fbc
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 > >
> > ---
> > 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
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 16/19] drm/i915: Use new atomic iterator macros in fbc
On Mon, Oct 17, 2016 at 02:37:15PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst
> ---
> 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
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 16/19] drm/i915: Use new atomic iterator macros in fbc
Signed-off-by: Maarten Lankhorst
---
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;
- 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);
--
2.7.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
