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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Dec 17, 2019 at 09:37:03AM -0800, Lucas De Marchi wrote:
> On Tue, Dec 17, 2019 at 08:39:15AM -0800, Jose Souza wrote:
> >On Mon, 2019-12-16 at 15:52 -0800, Lucas De Marchi wrote:
> >> On Mon, Dec 16, 2019 at 02:07:37PM -0800, Jose 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 moved it to a
> >> > better
> >> > place intel_atomic.c.
> >> >
> >> > That allowed us to convert the whole
> >> > intel_encoders_update_prepare/complete to intel types too.
> >> >
> >> > No behavior changes intended here.
> >> >
> >> > v3:
> >> > - removed digital from exported version of
> >> > intel_connector_needs_modeset
> >> > - rollback connector to drm type
> >> >
> >> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> >> > ---
> >> > drivers/gpu/drm/i915/display/intel_atomic.c  | 18 +++++++
> >> > drivers/gpu/drm/i915/display/intel_atomic.h  |  2 +
> >> > drivers/gpu/drm/i915/display/intel_display.c | 53 ++++++-----------
> >> > ---
> >> > 3 files changed, 36 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..b7dda18b6f29 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> >> > @@ -174,6 +174,24 @@ intel_digital_connector_duplicate_state(struct
> >> > drm_connector *connector)
> >> > 	return &state->base;
> >> > }
> >> >
> >> > +/**
> >> > + * intel_connector_needs_modeset - check if connector needs a
> >> > modeset
> >> > + */
> >> > +bool
> >> > +intel_connector_needs_modeset(struct intel_atomic_state *state,
> >> > +			      struct drm_connector *connector)
> >> > +{
> >> > +	const struct drm_connector_state *old_conn_state,
> >> > *new_conn_state;
> >> > +
> >> > +	old_conn_state = drm_atomic_get_old_connector_state(&state-
> >> > >base, connector);
> >> > +	new_conn_state = drm_atomic_get_new_connector_state(&state-
> >> > >base, connector);
> >> > +
> >> > +	return old_conn_state->crtc != new_conn_state->crtc ||
> >> > +	       (new_conn_state->crtc &&
> >> > +		drm_atomic_crtc_needs_modeset(drm_atomic_get_new_crtc_s
> >> > tate(&state->base,
> >> > +									
> >> >     new_conn_state->crtc)));
> >> > +}
> >> > +
> >> > /**
> >> >  * 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..a7d1a8576c48 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> >> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> >> > @@ -32,6 +32,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_connector_needs_modeset(struct intel_atomic_state
> >> > *state,
> >> > +				   struct drm_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 8e3e05cfcb27..0ee2e86a8826 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;
> >>
> >> what's up with this fight new_conn_state vs new_connector_state? It'
> >> s
> >> the first time in the driver we would actually drive such conversion.
> >> I'd say to just keep the old name.
> >
> >Ville asked me to rename conn to connector in the new functions that
> >I'm adding as this function was heavily modified I did the same here.
> >I will rollback if agreed by you and Ville(the ones helping with TGL
> >MST)
> 
> I think that was about conn -> connector, not *conn_state ->
> *connector_state, wasn't it?

I think we generally have full 'connector' and abbreviated
'conn_state'. A bit inconsistent in itself but IMO still better
to stick to common naming conventions so the poor brain has an
easier job when reading code. We could perhaps do a
s/connector/conn/ over the whole codebase to make things a bit
shorter in a bunch of places.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx