Re: [PATCH 12/13] drm/i915: Manage HSW/BDW LCPLLs with the shared dpll interface

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Tue, 2016-03-08 at 11:11 +0000, Conselvan De Oliveira, Ander wrote:
> On Tue, 2016-03-08 at 12:05 +0100, Maarten Lankhorst wrote:
> > Op 26-02-16 om 14:54 schreef Ander Conselvan de Oliveira:
> > > Manage the LCPLLs used with DisplayPort, so that all the HSW/BDW DPLLs
> > > are managed by the shared dpll code.
> > > 
> > > Signed-off-by: Ander Conselvan de Oliveira <
> > > ander.conselvan.de.oliveira@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c      | 18 ++++----
> > >  drivers/gpu/drm/i915/intel_display.c  | 35 ++++++++++++----
> > >  drivers/gpu/drm/i915/intel_dp.c       | 23 +---------
> > >  drivers/gpu/drm/i915/intel_dp_mst.c   |  4 --
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 79
> > > +++++++++++++++++++++++++++++++-
> > > ---
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.h |  5 ++-
> > >  drivers/gpu/drm/i915/intel_drv.h      |  1 -
> > >  7 files changed, 110 insertions(+), 55 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index ad7888c..3cb9f36 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -992,17 +992,13 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
> > >  {
> > >  	struct intel_shared_dpll *pll;
> > >  
> > > -	if (intel_encoder->type == INTEL_OUTPUT_HDMI ||
> > > -	    intel_encoder->type == INTEL_OUTPUT_ANALOG) {
> > > -		pll = intel_get_shared_dpll(intel_crtc, crtc_state,
> > > -					    intel_encoder);
> > > -		if (!pll)
> > > -			DRM_DEBUG_DRIVER("failed to find PLL for pipe
> > > %c\n",
> > > -					 pipe_name(intel_crtc->pipe));
> > > -		return pll;
> > > -	} else {
> > > -		return true;
> > > -	}
> > > +	pll = intel_get_shared_dpll(intel_crtc, crtc_state,
> > > +				    intel_encoder);
> > > +	if (!pll)
> > > +		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> > > +				 pipe_name(intel_crtc->pipe));
> > > +
> > > +	return pll;
> > >  }
> > Eventually the reliance on intel_encoder->type should end here, and based on
> > the connector.
> 
> Do you mean that the parameter to get_shared_dpll() should be connector
> instead
> of encoder?
> 
> > It would fix some kms tests, but that would be better to do in a future
> > patch.
> > > ...
> > > 
> > > +static bool hsw_ddi_lcpll_get_hw_state(struct drm_i915_private *dev_priv,
> > > +				       struct intel_shared_dpll *pll,
> > > +				       struct intel_dpll_hw_state
> > > *hw_state)
> > > +{
> > > +	/*
> > > +	 * LC PLL is kept enabled all the time since it drives CDCLK. The
> > > +	 * state checker still expects it to be disabled when it is not
> > > used
> > > +	 * by any crtc. To avoid adding a case to LC PLL, just tell the
> > > +	 * state checker what it expects.
> > > +	 */
> > > +	if (pll->config.crtc_mask)
> > > +		return true;
> > > +	else
> > > +		return false;
> > > +}
> > Wouldn't it be better to return true or the real hardware state then, and
> > set
> > the ALWAYS_ON flag from the next patch?
> 
> That's exactly what v2 of this patch does. :)

Well, actually, I think I fumbled git add or something, so I need to send a v3
of that patch. I also have a v3 of the SKL DPLL0 one, now that I managed to test
it. So I'll resend the series later today.

Ander

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