[Intel-gfx] [PATCH] drm/i915: Fix DPLL warning when starting guest VM

[Zhao, Xinda]

Hi:

Thanks for your quickly response.
I have updated the comments in the following text

Thanks!

Best Wishes!
Xinda

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Monday, October 30, 2017 11:14 PM
> To: Zhao, Xinda <xinda.zhao at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Zhao at freedesktop.org;
> intel-gvt-dev at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix DPLL warning when starting
> guest VM
> 
> On Mon, Oct 30, 2017 at 03:49:28PM +0200, Ville Syrjälä wrote:
> > On Mon, Oct 30, 2017 at 04:17:06PM +0800, Zhao, Xinda wrote:
> > > The warning is occurred in guest VM when trying to get clock in
> > > encoder initialization.
> 
> What does guest VM mean here? gvt? If so, why do you claim to have an
> enabled port without an enabled pipe?

[xinda] 
Yes, gvt-g. 

We emulate a DP device on port B that is fixed to pipe A for each guest VM by setting following register, it is mandatory.
TRANS_DDI_FUNC_CTL(TRANSCODER_A)) |= (PORT_B << TRANS_DDI_PORT_SHIFT);

We don't emulate the status of pipe, whether it is enabled or not, it depends on the i915 setting in guest VM, it is optional.
The PIPECONF register will be trapped, but the behavior will not be emulated.

> 
> > >
> > > intel_modeset_init()
> > >   ->intel_modeset_setup_hw_state()
> > >     ->intel_ddi_get_config()
> > >       ->intel_ddi_clock_get()
> > >         ->skl_ddi_clock_get()
> > >           ->intel_get_shared_dpll_id()
> > >             ->WARN_ON(pll < dev_priv->shared_dplls||
> > >                 pll >
> > > &dev_priv->shared_dplls[dev_priv->num_shared_dpll])
> > >
> > > In encoder initialization, shared DPLL is used for calculating clock
> > > for DDI ports, but it is not set when crtc is not active.
> > > In some cases, encoder is enabled while crtc is disabled, during
> > > encoder initialization, the warning occurred.
> > >
> > > Signed-off-by: Zhao, Xinda <xinda.zhao at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 28c25cb..ef35c12 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -1489,6 +1489,16 @@ void intel_ddi_clock_get(struct intel_encoder
> *encoder,
> > >  			 struct intel_crtc_state *pipe_config)  {
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > +	struct intel_crtc *crtc = to_intel_crtc(pipe_config->base.crtc);
> > > +
> > > +	/*
> > > +	 * For DDI ports we always use a shared PLL.
> > > +	 * But the shared PLL will not be set when crtc is not active.
> > > +	 */
> > > +	if (crtc->active == false) {
> >
> > Seem to me that the correct fix would be to do the DPLL readout
> > regardless of the state of the pipe.

[xinda]
As many components in "struct intel_crtc_state pipe_config" are not reading out when crtc is inactive, like pixel_multiplier, fdi_lanes, fdi_m_n....,
should they all be read out?

I think the better choice is to add a judgement when the components are used, like my solution.


> >
> > > +		DRM_DEBUG_KMS("Trying to get clock, but pipe is not
> active.\n");
> > > +		return;
> > > +	}
> > >
> > >  	if (INTEL_GEN(dev_priv) <= 8)
> > >  		hsw_ddi_clock_get(encoder, pipe_config);
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC