Re: [PATCH] drm/i915/skl: Handle eDP from generic crtc_compute_clock vfunc

From: Ander Conselvan De Oliveira <conselvan2@gmail.com>
To: Damien Lespiau <damien.lespiau@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915/skl: Handle eDP from generic crtc_compute_clock vfunc
Date: Fri, 15 May 2015 13:31:10 +0300	[thread overview]
Message-ID: <1431685870.2600.18.camel@gmail.com> (raw)
In-Reply-To: <20150513165617.GE10669@strange.amr.corp.intel.com>

On Wed, 2015-05-13 at 17:56 +0100, Damien Lespiau wrote:
> On Wed, May 13, 2015 at 05:40:44PM +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Since commit 4978cc93d9ac240b435ce60431aef24239b4c270 started clearing
> > dpll state and recomputing it via crtc_compute_clock (and probably some
> > other commit which triggered pipe config checking), modesetting is now
> > constantly triggering warnings about dpll_hw_state.ctrl1 mismatch.
> > 
> > Reason is crtc_compute_clock calls skl_ddi_pll_select which does not do
> > anything for eDP, leaving the ctrl1 state at the default of zero.
> > 
> > This potentially hacky fix makes skl_ddi_pll_select call
> > skl_edp_set_pll_config which fixes the problem for me.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Nop! (at least I really don't think so).
> 
> As discussed on IRC, on DDI platforms, the (e)DP compute_config() does
> the private DPLL selection, and the ddi_pll_select() does the the same
> for shared DPLLs (I'm not saying that the end result we want, just how
> it works today). That split comes from the introduction of shared_dpll
> in the DDI PLL selection last summer.
> 
> Anyway, this means that, for SKL, dpll_hw_state is touched by the
> encoder's compute_config() for (e)DP.
> 
> I think we could just remove the memset() in 4978cc93 (maybe?), or try
> to unify a bit better and only have one place where we do PLL selection
> (which I assume is part of the bigger atomic plan). Not
> skl_edp_set_pll_config() in both compute_config() and ddi_pll_select()
> though.

The memset() was added to fix a similar warning on a different platform,
but I can't remember which one now. Perhaps a more immediate fix would
be to move the memset() down the call chain.

Long term I think we should remove all the assumptions of initial values
from the compute config path. Having only one place that selects PLL
would also be nice IMO.

Ander

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