Re: [PATCH 03/12] drm/i915: Add output_types bitmask into the crtc state

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 03/12] drm/i915: Add output_types bitmask into the crtc state
Date: Mon, 13 Jun 2016 19:24:15 +0300	[thread overview]
Message-ID: <20160613162415.GQ4329@intel.com> (raw)
In-Reply-To: <20160613142555.GC1338@phenom.ffwll.local>

On Mon, Jun 13, 2016 at 04:25:55PM +0200, Daniel Vetter wrote:
> On Wed, Jun 08, 2016 at 01:41:38PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Rather than looping through encoders to see which encoder types
> > are being driven by the pipe, add an output_types bitmask into
> > the crtc state and populate it prior to compute_config and during
> > state readout.
> > 
> > v2: Determine output_types before .compute_config() hooks are called
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Not sure whether this will mesh with LSPCON perfectly, but the atomic way
> is to pass connector_state and crtc_state to all encoder functions. That's
> at least how the atomic helpers in the core work, and it imo works well.
> i915 isn't there yet, but we want that anyway.
> 
> Once you have the connector_state, the connector is just a pointer away.
> And the functions which care can look up the connector type.
> 
> I'd like to go that direction since it would align i915 more with core
> atomic. And that misalignment is imo a big reason for why our transition
> is so super painful.

We want the bitmask anyway since we want to figure things out about
the output type in places where we have no connector state (eg. DPLL
code , or when when we'd have to deal with multiple connector states
(cloning).

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 51 +++++++++++++++---------------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
> >  2 files changed, 26 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 12ff79594bc1..eabace447b1c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -535,14 +535,7 @@ needs_modeset(struct drm_crtc_state *state)
> >   */
> >  bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type)
> >  {
> > -	struct drm_device *dev = crtc->base.dev;
> > -	struct intel_encoder *encoder;
> > -
> > -	for_each_encoder_on_crtc(dev, &crtc->base, encoder)
> > -		if (encoder->type == type)
> > -			return true;
> > -
> > -	return false;
> > +	return crtc->config->output_types & (1 << type);
> >  }
> >  
> >  /**
> > @@ -552,28 +545,9 @@ bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type)
> >   * encoder->crtc.
> >   */
> >  static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state,
> > -				      int type)
> > +				      enum intel_output_type type)
> >  {
> > -	struct drm_atomic_state *state = crtc_state->base.state;
> > -	struct drm_connector *connector;
> > -	struct drm_connector_state *connector_state;
> > -	struct intel_encoder *encoder;
> > -	int i, num_connectors = 0;
> > -
> > -	for_each_connector_in_state(state, connector, connector_state, i) {
> > -		if (connector_state->crtc != crtc_state->base.crtc)
> > -			continue;
> > -
> > -		num_connectors++;
> > -
> > -		encoder = to_intel_encoder(connector_state->best_encoder);
> > -		if (encoder->type == type)
> > -			return true;
> > -	}
> > -
> > -	WARN_ON(num_connectors == 0);
> > -
> > -	return false;
> > +	return crtc_state->output_types & (1 << type);
> >  }
> >  
> >  /*
> > @@ -12529,6 +12503,19 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >  			       &pipe_config->pipe_src_w,
> >  			       &pipe_config->pipe_src_h);
> >  
> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> > +		if (connector_state->crtc != crtc)
> > +			continue;
> > +
> > +		encoder = to_intel_encoder(connector_state->best_encoder);
> > +
> > +		/*
> > +		 * Determine output_types before calling the .compute_config()
> > +		 * hooks so that the hooks can use this information safely.
> > +		 */
> > +		pipe_config->output_types |= 1 << encoder->type;
> > +	}
> > +
> >  encoder_retry:
> >  	/* Ensure the port clock defaults are reset when retrying. */
> >  	pipe_config->port_clock = 0;
> > @@ -12826,6 +12813,7 @@ intel_pipe_config_compare(struct drm_device *dev,
> >  		PIPE_CONF_CHECK_M_N_ALT(dp_m_n, dp_m2_n2);
> >  
> >  	PIPE_CONF_CHECK_I(has_dsi_encoder);
> > +	PIPE_CONF_CHECK_X(output_types);
> >  
> >  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
> >  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
> > @@ -13093,8 +13081,10 @@ verify_crtc_state(struct drm_crtc *crtc,
> >  				"Encoder connected to wrong pipe %c\n",
> >  				pipe_name(pipe));
> >  
> > -		if (active)
> > +		if (active) {
> > +			pipe_config->output_types |= 1 << encoder->type;
> >  			encoder->get_config(encoder, pipe_config);
> > +		}
> >  	}
> >  
> >  	if (!new_crtc_state->active)
> > @@ -15985,6 +15975,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  		if (encoder->get_hw_state(encoder, &pipe)) {
> >  			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> >  			encoder->base.crtc = &crtc->base;
> > +			crtc->config->output_types |= 1 << encoder->type;
> >  			encoder->get_config(encoder, crtc->config);
> >  		} else {
> >  			encoder->base.crtc = NULL;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index ebe7b3427e2e..3dae05e6d95b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -512,6 +512,11 @@ struct intel_crtc_state {
> >  	/* DSI has special cases */
> >  	bool has_dsi_encoder;
> >  
> > +	/* Bitmask of encoder types (enum intel_output_type)
> > +	 * driven by the pipe.
> > +	 */
> > +	unsigned int output_types;
> > +
> >  	/* Whether we should send NULL infoframes. Required for audio. */
> >  	bool has_hdmi_sink;
> >  
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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