[Intel-gfx] [PATCH 3/4] drm/i915: push crtc compute clock to encoder compute config on DDI

[Chauhan, Madhav]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
> Ville Syrjälä
> Sent: Thursday, October 12, 2017 9:59 PM
> To: Nikula, Jani <jani.nikula at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915: push crtc compute clock to
> encoder compute config on DDI
> 
> On Thu, Oct 12, 2017 at 07:05:32PM +0300, Jani Nikula wrote:
> > With this, we can get rid of the encoder specific stuff that we had in
> > haswell_crtc_compute_clock(), which we throw away entirely. This also
> > paves the way for potential further untangling of encoder specific
> > checks in the depths of intel_get_shared_dpll() that plague the DDI
> > platforms.
> >
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_crt.c     |  7 +++++++
> >  drivers/gpu/drm/i915/intel_ddi.c     |  7 +++++++
> >  drivers/gpu/drm/i915/intel_display.c | 21 ---------------------
> >  3 files changed, 14 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c
> > b/drivers/gpu/drm/i915/intel_crt.c
> > index 437339f5d098..67f771f24608 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -361,6 +361,7 @@ static bool hsw_crt_compute_config(struct
> intel_encoder *encoder,
> >  				   struct drm_connector_state *conn_state)  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	struct intel_crtc *crtc = to_intel_crtc(pipe_config->base.crtc);
> >
> >  	pipe_config->has_pch_encoder = true;
> >
> > @@ -377,6 +378,12 @@ static bool hsw_crt_compute_config(struct
> intel_encoder *encoder,
> >  	/* FDI must always be 2.7 GHz */
> >  	pipe_config->port_clock = 135000 * 2;
> >
> > +	if (!intel_get_shared_dpll(crtc, pipe_config, encoder)) {
> > +		DRM_DEBUG_KMS("failed to find PLL for pipe %c\n",
> > +			      pipe_name(crtc->pipe));
> > +		return false;
> > +	}
> 
> I'm not convinced we can do this. intel_modeset_clear_plls() hasn't even
> been called yet.

Agree. 
How about calling intel_modeset_clear_plls() before encoder->compute_config()
Inside intel_modeset_pipe_config() instead of intel_modeset_checks()??

Looked at modeset code briefly, couldn’t find any user of DPLL between intel_modeset_pipe_config() and
intel_modeset_checks().

Please correct if wrong.

Regards,
Madhav

> 
> > +
> >  	return true;
> >  }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index b307b6fe1ce3..9819e51fa160 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2590,6 +2590,7 @@ static bool intel_ddi_compute_config(struct
> intel_encoder *encoder,
> >  				     struct drm_connector_state *conn_state)
> {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > +	struct intel_crtc *crtc = to_intel_crtc(pipe_config->base.crtc);
> >  	int type = encoder->type;
> >  	int port = intel_ddi_get_encoder_port(encoder);
> >  	int ret;
> > @@ -2609,6 +2610,12 @@ static bool intel_ddi_compute_config(struct
> intel_encoder *encoder,
> >  			bxt_ddi_phy_calc_lane_lat_optim_mask(encoder,
> >  							     pipe_config-
> >lane_count);
> >
> > +	if (!intel_get_shared_dpll(crtc, pipe_config, encoder)) {
> > +		DRM_DEBUG_KMS("failed to find PLL for pipe %c\n",
> > +			      pipe_name(crtc->pipe));
> > +		return false;
> > +	}
> > +
> >  	return ret;
> >
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 2faa938f57a3..6ed299670f27 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9011,23 +9011,6 @@ void hsw_disable_pc8(struct drm_i915_private
> *dev_priv)
> >  	}
> >  }
> >
> > -static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
> > -				      struct intel_crtc_state *crtc_state)
> > -{
> > -	if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI)) {
> > -		struct intel_encoder *encoder =
> > -			intel_ddi_get_crtc_new_encoder(crtc_state);
> > -
> > -		if (!intel_get_shared_dpll(crtc, crtc_state, encoder)) {
> > -			DRM_DEBUG_DRIVER("failed to find PLL for pipe
> %c\n",
> > -					 pipe_name(crtc->pipe));
> > -			return -EINVAL;
> > -		}
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> >  static void cannonlake_get_ddi_pll(struct drm_i915_private *dev_priv,
> >  				   enum port port,
> >  				   struct intel_crtc_state *pipe_config) @@ -
> 14141,16 +14124,12
> > @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
> >  		dev_priv->display.get_pipe_config =
> haswell_get_pipe_config;
> >  		dev_priv->display.get_initial_plane_config =
> >  			skylake_get_initial_plane_config;
> > -		dev_priv->display.crtc_compute_clock =
> > -			haswell_crtc_compute_clock;
> >  		dev_priv->display.crtc_enable = haswell_crtc_enable;
> >  		dev_priv->display.crtc_disable = haswell_crtc_disable;
> >  	} else if (HAS_DDI(dev_priv)) {
> >  		dev_priv->display.get_pipe_config =
> haswell_get_pipe_config;
> >  		dev_priv->display.get_initial_plane_config =
> >  			ironlake_get_initial_plane_config;
> > -		dev_priv->display.crtc_compute_clock =
> > -			haswell_crtc_compute_clock;
> >  		dev_priv->display.crtc_enable = haswell_crtc_enable;
> >  		dev_priv->display.crtc_disable = haswell_crtc_disable;
> >  	} else if (HAS_PCH_SPLIT(dev_priv)) {
> > --
> > 2.11.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx