[Intel-gfx] [PATCH v2 rebased 06/11] drm/i915/display: Share intel_connector_needs_modeset()

[Souza, Jose]

On Thu, 2019-12-12 at 17:52 +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 10:45:21AM -0800, José Roberto de Souza
> wrote:
> > intel_connector_needs_modeset() will be used outside of
> > intel_display.c in a future patch so it would only be necessary to
> > remove the state and add the prototype to the header file.
> > 
> > But while at it, I simplified the arguments and changed to intel
> > types and moved it to a better place intel_atomic.c.
> > 
> > That allowed us to convert the whole
> > intel_encoders_update_prepare/complete to intel type too.
> > 
> > No behavior changes intended here.
> > 
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_atomic.c  | 32 ++++++++++++
> >  drivers/gpu/drm/i915/display/intel_atomic.h  |  3 ++
> >  drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++--------
> > ------
> >  3 files changed, 51 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index fd0026fc3618..6e93a39a6fec 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -174,6 +174,38 @@ intel_digital_connector_duplicate_state(struct
> > drm_connector *connector)
> >  	return &state->base;
> >  }
> >  
> > +/**
> > + * intel_digital_connector_needs_modeset - check if connector
> > needs a modeset
> > + */
> > +bool
> > +intel_digital_connector_needs_modeset(struct intel_atomic_state
> > *state,
> 
> Why "digital"? Oh because intel_atomic_get_old_connector_state()
> return
> a ditgital_connector_state. A bit surprising.

All the exported functions that handles intel_digital_connector in
intel_atomic.h have digital in the name, just keeping it consistent.

> 
> I suggest using just drm_connector_state here to keep this function
> totally generic.
> 
> > +				      struct intel_connector
> > *connector)
> > +{
> > +	struct intel_digital_connector_state *old_connector_state,
> > *new_connector_state;
> > +	struct intel_crtc *old_crtc, *new_crtc;
> > +	struct intel_crtc_state *new_crtc_state;
> > +
> > +	old_connector_state =
> > intel_atomic_get_old_connector_state(state,
> > +								   conn
> > ector);
> 
> Could be done when declaring the variable. Dunno which is prettier
> though.

Mostly avoiding lines over 80 columns

> 
> > +	if (old_connector_state->base.crtc)
> > +		old_crtc = to_intel_crtc(old_connector_state-
> > >base.crtc);
> > +	else
> > +		old_crtc = NULL;
> 
> Simple
> old_crtc = to_intel_crtc(old_connector_state->base.crtc); 
> will do. Can be done when declaring the variable as well.
> 
> > +
> > +	new_connector_state =
> > intel_atomic_get_new_connector_state(state,
> > +								   conn
> > ector);
> > +	if (new_connector_state->base.crtc) {
> > +		new_crtc = to_intel_crtc(new_connector_state-
> > >base.crtc);
> 
> ditto.
> 
> > +		new_crtc_state = intel_atomic_get_new_crtc_state(state,
> > new_crtc);
> 
> Then this just becomes
> if (new_crtc)
> 	new_crtc_state = ...;
> 
> Or maybe
> new_crtc_state = new_crtc ? get : NULL;
> but that could be a bit ugly.
> 
> 
> > +	} else {
> > +		new_crtc_state = NULL;
> > +		new_crtc = NULL;
> > +	}
> > +
> > +	return new_crtc != old_crtc ||
> > +	       (new_crtc &&
> > drm_atomic_crtc_needs_modeset(&new_crtc_state->uapi));
> 
> Hmm. In fact this function could be one of those special cases where
> we
> might even want to use all drm_ types internally since we don't
> actually
> need anything else.

I'm fine in both ways, so I will be going back to drm_ types and
removing the digital from the function name.

> 
> > +}
> > +
> >  /**
> >   * intel_crtc_duplicate_state - duplicate crtc state
> >   * @crtc: drm crtc
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h
> > b/drivers/gpu/drm/i915/display/intel_atomic.h
> > index 7b49623419ba..ba9cc29a5865 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > @@ -17,6 +17,7 @@ struct drm_device;
> >  struct drm_i915_private;
> >  struct drm_property;
> >  struct intel_atomic_state;
> > +struct intel_connector;
> >  struct intel_crtc;
> >  struct intel_crtc_state;
> >  
> > @@ -32,6 +33,8 @@ int intel_digital_connector_atomic_check(struct
> > drm_connector *conn,
> >  					 struct drm_atomic_state
> > *state);
> >  struct drm_connector_state *
> >  intel_digital_connector_duplicate_state(struct drm_connector
> > *connector);
> > +bool intel_digital_connector_needs_modeset(struct
> > intel_atomic_state *state,
> > +					   struct intel_connector
> > *connector);
> >  
> >  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc
> > *crtc);
> >  void intel_crtc_destroy_state(struct drm_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index b4e44d3cd275..39b00a19d752 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6185,71 +6185,50 @@ intel_connector_primary_encoder(struct
> > intel_connector *connector)
> >  	return encoder;
> >  }
> >  
> > -static bool
> > -intel_connector_needs_modeset(struct intel_atomic_state *state,
> > -			      const struct drm_connector_state
> > *old_conn_state,
> > -			      const struct drm_connector_state
> > *new_conn_state)
> > -{
> > -	struct intel_crtc *old_crtc = old_conn_state->crtc ?
> > -				      to_intel_crtc(old_conn_state-
> > >crtc) : NULL;
> > -	struct intel_crtc *new_crtc = new_conn_state->crtc ?
> > -				      to_intel_crtc(new_conn_state-
> > >crtc) : NULL;
> > -
> > -	return new_crtc != old_crtc ||
> > -	       (new_crtc &&
> > -		needs_modeset(intel_atomic_get_new_crtc_state(state,
> > new_crtc)));
> > -}
> > -
> >  static void intel_encoders_update_prepare(struct
> > intel_atomic_state *state)
> >  {
> > -	struct drm_connector_state *old_conn_state;
> > -	struct drm_connector_state *new_conn_state;
> > -	struct drm_connector *conn;
> > +	struct intel_digital_connector_state *new_connector_state;
> > +	struct intel_connector *connector;
> >  	int i;
> >  
> > -	for_each_oldnew_connector_in_state(&state->base, conn,
> > -					   old_conn_state,
> > new_conn_state, i) {
> > +	for_each_new_intel_connector_in_state(state, connector,
> > +					      new_connector_state, i) {
> >  		struct intel_encoder *encoder;
> >  		struct intel_crtc *crtc;
> >  
> > -		if (!intel_connector_needs_modeset(state,
> > -						   old_conn_state,
> > -						   new_conn_state))
> > +		if (!intel_digital_connector_needs_modeset(state,
> > connector))
> >  			continue;
> >  
> > -		encoder =
> > intel_connector_primary_encoder(to_intel_connector(conn));
> > +		encoder = intel_connector_primary_encoder(connector);
> >  		if (!encoder->update_prepare)
> >  			continue;
> >  
> > -		crtc = new_conn_state->crtc ?
> > -			to_intel_crtc(new_conn_state->crtc) : NULL;
> > +		crtc = new_connector_state->base.crtc ?
> > +			to_intel_crtc(new_connector_state->base.crtc) :
> > NULL;
> >  		encoder->update_prepare(state, encoder, crtc);
> >  	}
> >  }
> >  
> >  static void intel_encoders_update_complete(struct
> > intel_atomic_state *state)
> >  {
> > -	struct drm_connector_state *old_conn_state;
> > -	struct drm_connector_state *new_conn_state;
> > -	struct drm_connector *conn;
> > +	struct intel_digital_connector_state *new_connector_state;
> > +	struct intel_connector *connector;
> >  	int i;
> >  
> > -	for_each_oldnew_connector_in_state(&state->base, conn,
> > -					   old_conn_state,
> > new_conn_state, i) {
> > +	for_each_new_intel_connector_in_state(state, connector,
> > +					      new_connector_state, i) {
> >  		struct intel_encoder *encoder;
> >  		struct intel_crtc *crtc;
> >  
> > -		if (!intel_connector_needs_modeset(state,
> > -						   old_conn_state,
> > -						   new_conn_state))
> > +		if (!intel_digital_connector_needs_modeset(state,
> > connector))
> >  			continue;
> >  
> > -		encoder =
> > intel_connector_primary_encoder(to_intel_connector(conn));
> > +		encoder = intel_connector_primary_encoder(connector);
> >  		if (!encoder->update_complete)
> >  			continue;
> >  
> > -		crtc = new_conn_state->crtc ?
> > -			to_intel_crtc(new_conn_state->crtc) : NULL;
> > +		crtc = new_connector_state->base.crtc ?
> > +			to_intel_crtc(new_connector_state->base.crtc) :
> > NULL;
> >  		encoder->update_complete(state, encoder, crtc);
> >  	}
> >  }
> > -- 
> > 2.24.1