Re: [PATCH] drm/i915: Nuke drm_crtc_state and use intel_atomic_state instead

[Manasi Navare <manasi.d.navare@intel.com>]

On Fri, Apr 12, 2019 at 09:11:58AM +0300, Ville Syrjälä wrote:
> On Thu, Apr 11, 2019 at 04:02:54PM -0700, Manasi Navare wrote:
> > This is one of the patches to start replacing drm pointers
> > and use the intel_atomic_state and intel_crtc to derive
> > the necessary intel state variables required for the intel
> > modeset functions.
> > 
> > Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++---------------
> >  1 file changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f29a348e8d71..062b9e86a987 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -125,8 +125,8 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
> >  			    const struct intel_crtc_state *pipe_config);
> >  static void chv_prepare_pll(struct intel_crtc *crtc,
> >  			    const struct intel_crtc_state *pipe_config);
> > -static void intel_begin_crtc_commit(struct drm_crtc *, struct drm_crtc_state *);
> > -static void intel_finish_crtc_commit(struct drm_crtc *, struct drm_crtc_state *);
> > +static void intel_begin_crtc_commit(struct intel_crtc *, struct intel_atomic_state *);
> > +static void intel_finish_crtc_commit(struct intel_crtc *, struct intel_atomic_state *);
> >  static void intel_crtc_init_scalers(struct intel_crtc *crtc,
> >  				    struct intel_crtc_state *crtc_state);
> >  static void skylake_pfit_enable(const struct intel_crtc_state *crtc_state);
> > @@ -13273,14 +13273,14 @@ static void intel_update_crtc(struct drm_crtc *crtc,
> >  	else if (new_plane_state)
> >  		intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
> >  
> > -	intel_begin_crtc_commit(crtc, old_crtc_state);
> > +	intel_begin_crtc_commit(intel_crtc, to_intel_atomic_state(state));
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9)
> >  		skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
> >  	else
> >  		i9xx_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
> >  
> > -	intel_finish_crtc_commit(crtc, old_crtc_state);
> > +	intel_finish_crtc_commit(intel_crtc, to_intel_atomic_state(state));
> >  }
> >  
> >  static void intel_update_crtcs(struct drm_atomic_state *state)
> > @@ -14070,18 +14070,16 @@ skl_max_scale(const struct intel_crtc_state *crtc_state,
> >  	return max_scale;
> >  }
> >  
> > -static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> > -				    struct drm_crtc_state *old_crtc_state)
> > +static void intel_begin_crtc_commit(struct intel_crtc *crtc,
> > +				    struct intel_atomic_state *state)
> 
> Flip the arguments around to be consistent with existing code.

But most functions have the modeset object (crtc, plane etc) as the first argument
then the state.
Do you still want to flip the arguments?

Manasi

> 
> >  {
> > -	struct drm_device *dev = crtc->dev;
> > +	struct drm_device *dev = crtc->base.dev;
> 
> That can be killed as well.
> 
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct intel_crtc_state *old_intel_cstate =
> 
> Please rename to old_crtc_state, and similarly for new_crtc_state.
> 
> > -		to_intel_crtc_state(old_crtc_state);
> > -	struct intel_atomic_state *old_intel_state =
> > -		to_intel_atomic_state(old_crtc_state->state);
> > +		intel_atomic_get_old_crtc_state(state,
> > +						crtc);
> 
> Too many newlines.
> 
> >  	struct intel_crtc_state *intel_cstate =
> > -		intel_atomic_get_new_crtc_state(old_intel_state, intel_crtc);
> > +		intel_atomic_get_new_crtc_state(state, crtc);
> >  	bool modeset = needs_modeset(&intel_cstate->base);
> >  
> >  	/* Perform vblank evasion around commit operation */
> > @@ -14101,7 +14099,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >  
> >  out:
> >  	if (dev_priv->display.atomic_update_watermarks)
> > -		dev_priv->display.atomic_update_watermarks(old_intel_state,
> > +		dev_priv->display.atomic_update_watermarks(state,
> >  							   intel_cstate);
> >  }
> >  
> > @@ -14121,21 +14119,21 @@ void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc,
> >  	}
> >  }
> >  
> > -static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> > -				     struct drm_crtc_state *old_crtc_state)
> > +static void intel_finish_crtc_commit(struct intel_crtc *crtc,
> > +				     struct intel_atomic_state *state)
> >  {
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	struct intel_atomic_state *old_intel_state =
> > -		to_intel_atomic_state(old_crtc_state->state);
> > +	struct drm_crtc_state *old_crtc_state =
> 
> Pls convert this to intel type as well.
> 
> > +		&intel_atomic_get_old_crtc_state(state,
> > +						crtc)->base;
> >  	struct intel_crtc_state *new_crtc_state =
> > -		intel_atomic_get_new_crtc_state(old_intel_state, intel_crtc);
> > +		intel_atomic_get_new_crtc_state(state, crtc);
> >  
> >  	intel_pipe_update_end(new_crtc_state);
> >  
> >  	if (new_crtc_state->update_pipe &&
> >  	    !needs_modeset(&new_crtc_state->base) &&
> >  	    old_crtc_state->mode.private_flags & I915_MODE_FLAG_INHERITED)
> > -		intel_crtc_arm_fifo_underrun(intel_crtc, new_crtc_state);
> > +		intel_crtc_arm_fifo_underrun(crtc, new_crtc_state);
> >  }
> >  
> >  /**
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx