Re: [Intel-gfx] [PATCH 7/9] drm/i915: Switch fbc over to for_each_new_intel_plane_in_state()

[Ville Syrjälä]

On Thu, Oct 12, 2017 at 09:21:07PM +0200, Daniel Vetter wrote:
> On Wed, Oct 11, 2017 at 07:04:53PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > Stop using the old for_each_intel_plane_in_state() type iteration
> > macro and replace it with for_each_new_intel_plane_in_state().
> > And similarly replace drm_atomic_get_existing_crtc_state() with
> > intel_atomic_get_new_crtc_state(). Switch over to intel_ types
> > as well to make the code less cluttered.
> > 
> > Cc: Maarten Lankhorst 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  7 +++
> >  drivers/gpu/drm/i915/intel_display.c |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h |  2 +-
> >  drivers/gpu/drm/i915/intel_fbc.c | 23 ++-
> >  4 files changed, 15 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index df120a38ae42..9c4735da2169 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -561,13 +561,13 @@ struct i915_hotplug {
> > for_each_power_well_rev(__dev_priv, __power_well)   
> > \
> > for_each_if ((__power_well)->domains & (__domain_mask))
> >  
> > -#define for_each_intel_plane_in_state(__state, plane, plane_state, __i) \
> > +#define for_each_new_intel_plane_in_state(__state, plane, new_plane_state, 
> > __i) \
> > for ((__i) = 0; \
> >  (__i) < (__state)->base.dev->mode_config.num_total_plane && \
> >  ((plane) = 
> > to_intel_plane((__state)->base.planes[__i].ptr), \
> > - (plane_state) = 
> > to_intel_plane_state((__state)->base.planes[__i].state), 1); \
> > + (new_plane_state) = 
> > to_intel_plane_state((__state)->base.planes[__i].new_state), 1); \
> >  (__i)++) \
> > -   for_each_if (plane_state)
> > +   for_each_if (plane)
> >  
> >  #define for_each_new_intel_crtc_in_state(__state, crtc, new_crtc_state, 
> > __i) \
> > for ((__i) = 0; \
> > @@ -577,7 +577,6 @@ struct i915_hotplug {
> >  (__i)++) \
> > for_each_if (crtc)
> 
> Would be nice to co-evolve our versions of these along the lines of
> 
> commit 331494eb51002d0ee99414e3918e06d5e9a3962d
> Author: Maarten Lankhorst 
> Date:   Wed Sep 27 10:35:32 2017 +0200
> 
> drm/atomic: Make atomic iterators less surprising

Indeed. The same idea popped into my head when I saw Maarten's patch,
but apparently I forgot to reply to the patch to propose it.

> 
> On the patch itself:
> 
> Reviewed-by: Daniel Vetter 
> 
> >  
> > -
> >  #define for_each_oldnew_intel_plane_in_state(__state, plane, 
> > old_plane_state, new_plane_state, __i) \
> > for ((__i) = 0; \
> >  (__i) < (__state)->base.dev->mode_config.num_total_plane && \
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 82be2342d1c6..ccdfd922fe5b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12025,7 +12025,7 @@ static int intel_atomic_check(struct drm_device 
> > *dev,
> > if (ret)
> > return ret;
> >  
> > -   intel_fbc_choose_crtc(dev_priv, state);
> > +   intel_fbc_choose_crtc(dev_priv, intel_state);
> > return calc_watermark_data(state);
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 08318260453b..0852b33712b1 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1645,7 +1645,7 @@ static inline void intel_fbdev_restore_mode(struct 
> > drm_device *dev)
> >  
> >  /* intel_fbc.c */
> >  void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> > -  struct drm_atomic_state *state);
> > +  struct intel_atomic_state *state);
> >  bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
> >  void intel_fbc_pre_update(struct intel_crtc *crtc,
> >   struct intel_crtc_state *crtc_state,
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c 
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index 8e3a05505f49..0b40b89f8e2b 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -1051,11 +1051,11 @@ void intel_fbc_flush(struct drm_i915_private 
> > *dev_priv,
> >   * enable FBC for the chosen CRTC. If it does, it will set 
> > dev_priv->fbc.crtc.
> >   */
> >  void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> > -  struct drm_atomic_state *state)
> > +  struct intel_atomic_state *state)
> >  {
> > struct intel_fbc *fbc = _priv->fbc;
> > -   struct drm_plane *plane;
> > -   struct drm_plane_state *plane_state;
> > +   struct 

Re: [Intel-gfx] [PATCH 7/9] drm/i915: Switch fbc over to for_each_new_intel_plane_in_state()

[Daniel Vetter]

On Wed, Oct 11, 2017 at 07:04:53PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Stop using the old for_each_intel_plane_in_state() type iteration
> macro and replace it with for_each_new_intel_plane_in_state().
> And similarly replace drm_atomic_get_existing_crtc_state() with
> intel_atomic_get_new_crtc_state(). Switch over to intel_ types
> as well to make the code less cluttered.
> 
> Cc: Maarten Lankhorst 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  7 +++
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h |  2 +-
>  drivers/gpu/drm/i915/intel_fbc.c | 23 ++-
>  4 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index df120a38ae42..9c4735da2169 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -561,13 +561,13 @@ struct i915_hotplug {
>   for_each_power_well_rev(__dev_priv, __power_well)   
> \
>   for_each_if ((__power_well)->domains & (__domain_mask))
>  
> -#define for_each_intel_plane_in_state(__state, plane, plane_state, __i) \
> +#define for_each_new_intel_plane_in_state(__state, plane, new_plane_state, 
> __i) \
>   for ((__i) = 0; \
>(__i) < (__state)->base.dev->mode_config.num_total_plane && \
>((plane) = 
> to_intel_plane((__state)->base.planes[__i].ptr), \
> -   (plane_state) = 
> to_intel_plane_state((__state)->base.planes[__i].state), 1); \
> +   (new_plane_state) = 
> to_intel_plane_state((__state)->base.planes[__i].new_state), 1); \
>(__i)++) \
> - for_each_if (plane_state)
> + for_each_if (plane)
>  
>  #define for_each_new_intel_crtc_in_state(__state, crtc, new_crtc_state, __i) 
> \
>   for ((__i) = 0; \
> @@ -577,7 +577,6 @@ struct i915_hotplug {
>(__i)++) \
>   for_each_if (crtc)

Would be nice to co-evolve our versions of these along the lines of

commit 331494eb51002d0ee99414e3918e06d5e9a3962d
Author: Maarten Lankhorst 
Date:   Wed Sep 27 10:35:32 2017 +0200

drm/atomic: Make atomic iterators less surprising

On the patch itself:

Reviewed-by: Daniel Vetter 

>  
> -
>  #define for_each_oldnew_intel_plane_in_state(__state, plane, 
> old_plane_state, new_plane_state, __i) \
>   for ((__i) = 0; \
>(__i) < (__state)->base.dev->mode_config.num_total_plane && \
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 82be2342d1c6..ccdfd922fe5b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12025,7 +12025,7 @@ static int intel_atomic_check(struct drm_device *dev,
>   if (ret)
>   return ret;
>  
> - intel_fbc_choose_crtc(dev_priv, state);
> + intel_fbc_choose_crtc(dev_priv, intel_state);
>   return calc_watermark_data(state);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 08318260453b..0852b33712b1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1645,7 +1645,7 @@ static inline void intel_fbdev_restore_mode(struct 
> drm_device *dev)
>  
>  /* intel_fbc.c */
>  void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> -struct drm_atomic_state *state);
> +struct intel_atomic_state *state);
>  bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
>  void intel_fbc_pre_update(struct intel_crtc *crtc,
> struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c 
> b/drivers/gpu/drm/i915/intel_fbc.c
> index 8e3a05505f49..0b40b89f8e2b 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1051,11 +1051,11 @@ void intel_fbc_flush(struct drm_i915_private 
> *dev_priv,
>   * enable FBC for the chosen CRTC. If it does, it will set 
> dev_priv->fbc.crtc.
>   */
>  void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> -struct drm_atomic_state *state)
> +struct intel_atomic_state *state)
>  {
>   struct intel_fbc *fbc = _priv->fbc;
> - struct drm_plane *plane;
> - struct drm_plane_state *plane_state;
> + struct intel_plane *plane;
> + struct intel_plane_state *plane_state;
>   bool crtc_chosen = false;
>   int i;
>  
> @@ -1063,7 +1063,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private 
> *dev_priv,
>  
>   /* Does this atomic commit involve the CRTC currently tied to FBC? */
>   if (fbc->crtc &&
> - !drm_atomic_get_existing_crtc_state(state, 

[Intel-gfx] [PATCH 7/9] drm/i915: Switch fbc over to for_each_new_intel_plane_in_state()

[Ville Syrjala]

From: Ville Syrjälä 

Stop using the old for_each_intel_plane_in_state() type iteration
macro and replace it with for_each_new_intel_plane_in_state().
And similarly replace drm_atomic_get_existing_crtc_state() with
intel_atomic_get_new_crtc_state(). Switch over to intel_ types
as well to make the code less cluttered.

Cc: Maarten Lankhorst 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_drv.h  |  7 +++
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h |  2 +-
 drivers/gpu/drm/i915/intel_fbc.c | 23 ++-
 4 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index df120a38ae42..9c4735da2169 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -561,13 +561,13 @@ struct i915_hotplug {
for_each_power_well_rev(__dev_priv, __power_well)   
\
for_each_if ((__power_well)->domains & (__domain_mask))
 
-#define for_each_intel_plane_in_state(__state, plane, plane_state, __i) \
+#define for_each_new_intel_plane_in_state(__state, plane, new_plane_state, 
__i) \
for ((__i) = 0; \
 (__i) < (__state)->base.dev->mode_config.num_total_plane && \
 ((plane) = 
to_intel_plane((__state)->base.planes[__i].ptr), \
- (plane_state) = 
to_intel_plane_state((__state)->base.planes[__i].state), 1); \
+ (new_plane_state) = 
to_intel_plane_state((__state)->base.planes[__i].new_state), 1); \
 (__i)++) \
-   for_each_if (plane_state)
+   for_each_if (plane)
 
 #define for_each_new_intel_crtc_in_state(__state, crtc, new_crtc_state, __i) \
for ((__i) = 0; \
@@ -577,7 +577,6 @@ struct i915_hotplug {
 (__i)++) \
for_each_if (crtc)
 
-
 #define for_each_oldnew_intel_plane_in_state(__state, plane, old_plane_state, 
new_plane_state, __i) \
for ((__i) = 0; \
 (__i) < (__state)->base.dev->mode_config.num_total_plane && \
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 82be2342d1c6..ccdfd922fe5b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12025,7 +12025,7 @@ static int intel_atomic_check(struct drm_device *dev,
if (ret)
return ret;
 
-   intel_fbc_choose_crtc(dev_priv, state);
+   intel_fbc_choose_crtc(dev_priv, intel_state);
return calc_watermark_data(state);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 08318260453b..0852b33712b1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1645,7 +1645,7 @@ static inline void intel_fbdev_restore_mode(struct 
drm_device *dev)
 
 /* intel_fbc.c */
 void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
-  struct drm_atomic_state *state);
+  struct intel_atomic_state *state);
 bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
 void intel_fbc_pre_update(struct intel_crtc *crtc,
  struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 8e3a05505f49..0b40b89f8e2b 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1051,11 +1051,11 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
  * enable FBC for the chosen CRTC. If it does, it will set dev_priv->fbc.crtc.
  */
 void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
-  struct drm_atomic_state *state)
+  struct intel_atomic_state *state)
 {
struct intel_fbc *fbc = _priv->fbc;
-   struct drm_plane *plane;
-   struct drm_plane_state *plane_state;
+   struct intel_plane *plane;
+   struct intel_plane_state *plane_state;
bool crtc_chosen = false;
int i;
 
@@ -1063,7 +1063,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private 
*dev_priv,
 
/* Does this atomic commit involve the CRTC currently tied to FBC? */
if (fbc->crtc &&
-   !drm_atomic_get_existing_crtc_state(state, >crtc->base))
+   !intel_atomic_get_new_crtc_state(state, fbc->crtc))
goto out;
 
if (!intel_fbc_can_enable(dev_priv))
@@ -1073,13 +1073,11 @@ 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_new_plane_in_state(state, plane, plane_state, i) {
-   struct intel_plane_state *intel_plane_state =
-