* [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw @ 2015-09-23 15:34 Gabriel Feceoru 2015-10-13 13:18 ` Maarten Lankhorst 0 siblings, 1 reply; 19+ messages in thread From: Gabriel Feceoru @ 2015-09-23 15:34 UTC (permalink / raw) To: intel-gfx Using 2 connectors (DVI and VGA) will cause wrpll to be set for INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA Supresses errors like these: [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll Signed-off-by: Gabriel Feceoru <gabriel.feceoru@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 4823184..1e5001a 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1245,6 +1245,9 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc, { int clock = crtc_state->port_clock; + memset(&crtc_state->dpll_hw_state, 0, + sizeof(crtc_state->dpll_hw_state)); + if (intel_encoder->type == INTEL_OUTPUT_HDMI) { struct intel_shared_dpll *pll; uint32_t val; @@ -1256,9 +1259,6 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc, WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) | WRPLL_DIVIDER_POST(p); - memset(&crtc_state->dpll_hw_state, 0, - sizeof(crtc_state->dpll_hw_state)); - crtc_state->dpll_hw_state.wrpll = val; pll = intel_get_shared_dpll(intel_crtc, crtc_state); -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-09-23 15:34 [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw Gabriel Feceoru @ 2015-10-13 13:18 ` Maarten Lankhorst 2015-10-13 13:35 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Maarten Lankhorst @ 2015-10-13 13:18 UTC (permalink / raw) To: Gabriel Feceoru, intel-gfx Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > Using 2 connectors (DVI and VGA) will cause wrpll to be set for > INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA > > Supresses errors like these: > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll > Looks like a good idea to always zero it. Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-10-13 13:18 ` Maarten Lankhorst @ 2015-10-13 13:35 ` Daniel Vetter 2015-10-13 13:43 ` Daniel Vetter 2015-10-13 13:43 ` Maarten Lankhorst 0 siblings, 2 replies; 19+ messages in thread From: Daniel Vetter @ 2015-10-13 13:35 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > > Using 2 connectors (DVI and VGA) will cause wrpll to be set for > > INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA > > > > Supresses errors like these: > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll > > > Looks like a good idea to always zero it. Except that we still have a bunch of cases where we recompute clock state but only partially. Can we just move them all up into a common place please? That would also catch cases where we simply forget to fill this out at all. One case I noticed is edp in skl_ddi_pll_select, but there's probably more. -Daniel > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-10-13 13:35 ` Daniel Vetter @ 2015-10-13 13:43 ` Daniel Vetter 2015-10-13 13:43 ` Maarten Lankhorst 1 sibling, 0 replies; 19+ messages in thread From: Daniel Vetter @ 2015-10-13 13:43 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx On Tue, Oct 13, 2015 at 03:35:01PM +0200, Daniel Vetter wrote: > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > > > Using 2 connectors (DVI and VGA) will cause wrpll to be set for > > > INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA > > > > > > Supresses errors like these: > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll > > > > > Looks like a good idea to always zero it. > > Except that we still have a bunch of cases where we recompute clock state > but only partially. Can we just move them all up into a common place > please? That would also catch cases where we simply forget to fill this > out at all. > > One case I noticed is edp in skl_ddi_pll_select, but there's probably > more. Specifically I think we should move the memset into intel_modeset_pipe_config and remove it from all the clock compute/pll select functions. Last time we wanted to do that it wasn't yet possible because the atomice modeset conversion and shared dpll tracking needed the old values, but that should be fixed now with crtc_state structures being completely free-standing. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-10-13 13:35 ` Daniel Vetter 2015-10-13 13:43 ` Daniel Vetter @ 2015-10-13 13:43 ` Maarten Lankhorst 2015-10-13 13:58 ` Daniel Vetter 1 sibling, 1 reply; 19+ messages in thread From: Maarten Lankhorst @ 2015-10-13 13:43 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx Op 13-10-15 om 15:35 schreef Daniel Vetter: > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: >> Op 23-09-15 om 17:34 schreef Gabriel Feceoru: >>> Using 2 connectors (DVI and VGA) will cause wrpll to be set for >>> INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA >>> >>> Supresses errors like these: >>> [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll >>> >> Looks like a good idea to always zero it. > Except that we still have a bunch of cases where we recompute clock state > but only partially. Can we just move them all up into a common place > please? That would also catch cases where we simply forget to fill this > out at all. > > One case I noticed is edp in skl_ddi_pll_select, but there's probably > more. > Something like below, with all the memsets for dpll_hw_state removed? diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 633da693fed8..956b7ffab32f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11911,7 +11911,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) { struct drm_crtc_state tmp_state; struct intel_crtc_scaler_state scaler_state; - struct intel_dpll_hw_state dpll_hw_state; enum intel_dpll_id shared_dpll; uint32_t ddi_pll_sel; bool force_thru; @@ -11924,7 +11923,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) tmp_state = crtc_state->base; scaler_state = crtc_state->scaler_state; shared_dpll = crtc_state->shared_dpll; - dpll_hw_state = crtc_state->dpll_hw_state; ddi_pll_sel = crtc_state->ddi_pll_sel; force_thru = crtc_state->pch_pfit.force_thru; _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-10-13 13:43 ` Maarten Lankhorst @ 2015-10-13 13:58 ` Daniel Vetter 2015-10-13 14:00 ` Maarten Lankhorst 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2015-10-13 13:58 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote: > Op 13-10-15 om 15:35 schreef Daniel Vetter: > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: > >> Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > >>> Using 2 connectors (DVI and VGA) will cause wrpll to be set for > >>> INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA > >>> > >>> Supresses errors like these: > >>> [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll > >>> > >> Looks like a good idea to always zero it. > > Except that we still have a bunch of cases where we recompute clock state > > but only partially. Can we just move them all up into a common place > > please? That would also catch cases where we simply forget to fill this > > out at all. > > > > One case I noticed is edp in skl_ddi_pll_select, but there's probably > > more. > > > Something like below, with all the memsets for dpll_hw_state removed? I think this will blow up since we recompute clock state only when needs_modeset is true. So needs a bit more intelligence in deciding when to clear it I think. -Daniel > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 633da693fed8..956b7ffab32f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11911,7 +11911,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > { > struct drm_crtc_state tmp_state; > struct intel_crtc_scaler_state scaler_state; > - struct intel_dpll_hw_state dpll_hw_state; > enum intel_dpll_id shared_dpll; > uint32_t ddi_pll_sel; > bool force_thru; > @@ -11924,7 +11923,6 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state) > tmp_state = crtc_state->base; > scaler_state = crtc_state->scaler_state; > shared_dpll = crtc_state->shared_dpll; > - dpll_hw_state = crtc_state->dpll_hw_state; > ddi_pll_sel = crtc_state->ddi_pll_sel; > force_thru = crtc_state->pch_pfit.force_thru; > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-10-13 13:58 ` Daniel Vetter @ 2015-10-13 14:00 ` Maarten Lankhorst 2015-10-13 14:08 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Maarten Lankhorst @ 2015-10-13 14:00 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx Op 13-10-15 om 15:58 schreef Daniel Vetter: > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote: >> Op 13-10-15 om 15:35 schreef Daniel Vetter: >>> On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: >>>> Op 23-09-15 om 17:34 schreef Gabriel Feceoru: >>>>> Using 2 connectors (DVI and VGA) will cause wrpll to be set for >>>>> INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA >>>>> >>>>> Supresses errors like these: >>>>> [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll >>>>> >>>> Looks like a good idea to always zero it. >>> Except that we still have a bunch of cases where we recompute clock state >>> but only partially. Can we just move them all up into a common place >>> please? That would also catch cases where we simply forget to fill this >>> out at all. >>> >>> One case I noticed is edp in skl_ddi_pll_select, but there's probably >>> more. >>> >> Something like below, with all the memsets for dpll_hw_state removed? > I think this will blow up since we recompute clock state only when > needs_modeset is true. So needs a bit more intelligence in deciding when > to clear it I think. Oops you're right. Maybe intel_modeset_clear_plls because that's where all the clock state belongs? _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-10-13 14:00 ` Maarten Lankhorst @ 2015-10-13 14:08 ` Daniel Vetter 2015-10-14 8:21 ` Ander Conselvan De Oliveira 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2015-10-13 14:08 UTC (permalink / raw) To: Maarten Lankhorst; +Cc: intel-gfx On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote: > Op 13-10-15 om 15:58 schreef Daniel Vetter: > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote: > >> Op 13-10-15 om 15:35 schreef Daniel Vetter: > >>> On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: > >>>> Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > >>>>> Using 2 connectors (DVI and VGA) will cause wrpll to be set for > >>>>> INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA > >>>>> > >>>>> Supresses errors like these: > >>>>> [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll > >>>>> > >>>> Looks like a good idea to always zero it. > >>> Except that we still have a bunch of cases where we recompute clock state > >>> but only partially. Can we just move them all up into a common place > >>> please? That would also catch cases where we simply forget to fill this > >>> out at all. > >>> > >>> One case I noticed is edp in skl_ddi_pll_select, but there's probably > >>> more. > >>> > >> Something like below, with all the memsets for dpll_hw_state removed? > > I think this will blow up since we recompute clock state only when > > needs_modeset is true. So needs a bit more intelligence in deciding when > > to clear it I think. > Oops you're right. Maybe intel_modeset_clear_plls because that's where all the clock state belongs? Yeah that might be an even better place, in the loop after the continue; statement. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-10-13 14:08 ` Daniel Vetter @ 2015-10-14 8:21 ` Ander Conselvan De Oliveira 2015-10-14 12:44 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Ander Conselvan De Oliveira @ 2015-10-14 8:21 UTC (permalink / raw) To: Daniel Vetter, Maarten Lankhorst; +Cc: intel-gfx On Tue, 2015-10-13 at 16:08 +0200, Daniel Vetter wrote: > On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote: > > Op 13-10-15 om 15:58 schreef Daniel Vetter: > > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote: > > > > Op 13-10-15 om 15:35 schreef Daniel Vetter: > > > > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: > > > > > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > > > > > > > Using 2 connectors (DVI and VGA) will cause wrpll to be set for > > > > > > > INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA > > > > > > > > > > > > > > Supresses errors like these: > > > > > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll > > > > > > > > > > > > > Looks like a good idea to always zero it. > > > > > Except that we still have a bunch of cases where we recompute clock state > > > > > but only partially. Can we just move them all up into a common place > > > > > please? That would also catch cases where we simply forget to fill this > > > > > out at all. > > > > > > > > > > One case I noticed is edp in skl_ddi_pll_select, but there's probably > > > > > more. > > > > > > > > > Something like below, with all the memsets for dpll_hw_state removed? > > > I think this will blow up since we recompute clock state only when > > > needs_modeset is true. So needs a bit more intelligence in deciding when > > > to clear it I think. > > Oops you're right. Maybe intel_modeset_clear_plls because that's where all the clock state > > belongs? > > Yeah that might be an even better place, in the loop after the continue; > statement. The reason I didn't put the memset there in the first place was the way we calculate plls for DP with DDI platforms. In that case, ddi_pll_sel is setup from the encoder_config instead of compute_clock, so a memset ends up clearing the new pll config. Ander _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-10-14 8:21 ` Ander Conselvan De Oliveira @ 2015-10-14 12:44 ` Daniel Vetter 2015-10-14 13:58 ` Ander Conselvan De Oliveira 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2015-10-14 12:44 UTC (permalink / raw) To: Ander Conselvan De Oliveira; +Cc: intel-gfx On Wed, Oct 14, 2015 at 11:21:46AM +0300, Ander Conselvan De Oliveira wrote: > On Tue, 2015-10-13 at 16:08 +0200, Daniel Vetter wrote: > > On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote: > > > Op 13-10-15 om 15:58 schreef Daniel Vetter: > > > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote: > > > > > Op 13-10-15 om 15:35 schreef Daniel Vetter: > > > > > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: > > > > > > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > > > > > > > > Using 2 connectors (DVI and VGA) will cause wrpll to be set for > > > > > > > > INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA > > > > > > > > > > > > > > > > Supresses errors like these: > > > > > > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll > > > > > > > > > > > > > > > Looks like a good idea to always zero it. > > > > > > Except that we still have a bunch of cases where we recompute clock state > > > > > > but only partially. Can we just move them all up into a common place > > > > > > please? That would also catch cases where we simply forget to fill this > > > > > > out at all. > > > > > > > > > > > > One case I noticed is edp in skl_ddi_pll_select, but there's probably > > > > > > more. > > > > > > > > > > > Something like below, with all the memsets for dpll_hw_state removed? > > > > I think this will blow up since we recompute clock state only when > > > > needs_modeset is true. So needs a bit more intelligence in deciding when > > > > to clear it I think. > > > Oops you're right. Maybe intel_modeset_clear_plls because that's where all the clock state > > > belongs? > > > > Yeah that might be an even better place, in the loop after the continue; > > statement. > > The reason I didn't put the memset there in the first place was the way we calculate plls for DP > with DDI platforms. In that case, ddi_pll_sel is setup from the encoder_config instead of > compute_clock, so a memset ends up clearing the new pll config. Hm, I forgot about this split totally. And there seems to be a giant mess going on here: In our top-level intel_atomic_check we have 4 parts to compute state: 1. drm_atomic_helper_check_modeset 2. intel_modeset_pipe_config 3. intel_modeset_checks 4. drm_atomic_helper_check_planes We recalculate clocks (by calling dev_priv->display.crtc_compute_clock) in 1., way ahead of anything else in intel_crtc_atomic_check. That looks very suspcious since it means only very later on (in the loop that does 2.) do we even decide whether we need to do a full modeset or not. So what I had in mind is that we clear clocks in intel_modeset_pipe_config, before we call any of the callbacks. That makes sure that when we decided to do a modeset, we do recompute the clocks correctly. First step would be to order 1 and 2 correctly I guess. Next up is 3, that one just has a confusing name - it doesn't clear PLL state, but updates the global pll state if necessary. So should perhaps be renamed to intel_modeset_compute_shared_dpll or similar. Related, there's also the scaler code in step 1, which is definitely bonkers since that's done before we recompute the config in step 3. But that's an unrelated issue really. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-10-14 12:44 ` Daniel Vetter @ 2015-10-14 13:58 ` Ander Conselvan De Oliveira 2015-10-14 15:03 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Ander Conselvan De Oliveira @ 2015-10-14 13:58 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Wed, 2015-10-14 at 14:44 +0200, Daniel Vetter wrote: > On Wed, Oct 14, 2015 at 11:21:46AM +0300, Ander Conselvan De Oliveira wrote: > > On Tue, 2015-10-13 at 16:08 +0200, Daniel Vetter wrote: > > > On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote: > > > > Op 13-10-15 om 15:58 schreef Daniel Vetter: > > > > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote: > > > > > > Op 13-10-15 om 15:35 schreef Daniel Vetter: > > > > > > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: > > > > > > > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > > > > > > > > > Using 2 connectors (DVI and VGA) will cause wrpll to be set for > > > > > > > > > INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA > > > > > > > > > > > > > > > > > > Supresses errors like these: > > > > > > > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll > > > > > > > > > > > > > > > > > Looks like a good idea to always zero it. > > > > > > > Except that we still have a bunch of cases where we recompute clock state > > > > > > > but only partially. Can we just move them all up into a common place > > > > > > > please? That would also catch cases where we simply forget to fill this > > > > > > > out at all. > > > > > > > > > > > > > > One case I noticed is edp in skl_ddi_pll_select, but there's probably > > > > > > > more. > > > > > > > > > > > > > Something like below, with all the memsets for dpll_hw_state removed? > > > > > I think this will blow up since we recompute clock state only when > > > > > needs_modeset is true. So needs a bit more intelligence in deciding when > > > > > to clear it I think. > > > > Oops you're right. Maybe intel_modeset_clear_plls because that's where all the clock state > > > > belongs? > > > > > > Yeah that might be an even better place, in the loop after the continue; > > > statement. > > > > The reason I didn't put the memset there in the first place was the way we calculate plls for DP > > with DDI platforms. In that case, ddi_pll_sel is setup from the encoder_config instead of > > compute_clock, so a memset ends up clearing the new pll config. > > Hm, I forgot about this split totally. And there seems to be a giant mess > going on here: > > In our top-level intel_atomic_check we have 4 parts to compute state: > 1. drm_atomic_helper_check_modeset > 2. intel_modeset_pipe_config > 3. intel_modeset_checks > 4. drm_atomic_helper_check_planes > > We recalculate clocks (by calling dev_priv->display.crtc_compute_clock) > in 1., way ahead of anything else in intel_crtc_atomic_check. That looks > very suspcious since it means only very later on (in the loop that does > 2.) do we even decide whether we need to do a full modeset or not. > > So what I had in mind is that we clear clocks in > intel_modeset_pipe_config, before we call any of the callbacks. That makes > sure that when we decided to do a modeset, we do recompute the clocks > correctly. I had a suspicion this would interact badly with how we "cancel" the modeset if the pipe config didn't changed, just after the call to intel_modeset_pipe_config(). It turns out there's an issue there already. There are two possibilities for the dpll_hw_state value after the new pipe_config is calculated. It may have the new values already for DP in HSW/BDW and eDP in SKL or it may still have the old value. In the latter case the new value is only calculated in .crtc_clock(), after we already compared the old and new configs and may have decided to skip the modeset. But doing the memset() in intel_modeset_pipe_config() would be find as long as we don't change our minds about doing a modeset later. Ander _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-10-14 13:58 ` Ander Conselvan De Oliveira @ 2015-10-14 15:03 ` Daniel Vetter 2015-11-10 12:53 ` Jani Nikula 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2015-10-14 15:03 UTC (permalink / raw) To: Ander Conselvan De Oliveira; +Cc: intel-gfx On Wed, Oct 14, 2015 at 04:58:55PM +0300, Ander Conselvan De Oliveira wrote: > On Wed, 2015-10-14 at 14:44 +0200, Daniel Vetter wrote: > > On Wed, Oct 14, 2015 at 11:21:46AM +0300, Ander Conselvan De Oliveira wrote: > > > On Tue, 2015-10-13 at 16:08 +0200, Daniel Vetter wrote: > > > > On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote: > > > > > Op 13-10-15 om 15:58 schreef Daniel Vetter: > > > > > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote: > > > > > > > Op 13-10-15 om 15:35 schreef Daniel Vetter: > > > > > > > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: > > > > > > > > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > > > > > > > > > > Using 2 connectors (DVI and VGA) will cause wrpll to be set for > > > > > > > > > > INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA > > > > > > > > > > > > > > > > > > > > Supresses errors like these: > > > > > > > > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll > > > > > > > > > > > > > > > > > > > Looks like a good idea to always zero it. > > > > > > > > Except that we still have a bunch of cases where we recompute clock state > > > > > > > > but only partially. Can we just move them all up into a common place > > > > > > > > please? That would also catch cases where we simply forget to fill this > > > > > > > > out at all. > > > > > > > > > > > > > > > > One case I noticed is edp in skl_ddi_pll_select, but there's probably > > > > > > > > more. > > > > > > > > > > > > > > > Something like below, with all the memsets for dpll_hw_state removed? > > > > > > I think this will blow up since we recompute clock state only when > > > > > > needs_modeset is true. So needs a bit more intelligence in deciding when > > > > > > to clear it I think. > > > > > Oops you're right. Maybe intel_modeset_clear_plls because that's where all the clock state > > > > > belongs? > > > > > > > > Yeah that might be an even better place, in the loop after the continue; > > > > statement. > > > > > > The reason I didn't put the memset there in the first place was the way we calculate plls for DP > > > with DDI platforms. In that case, ddi_pll_sel is setup from the encoder_config instead of > > > compute_clock, so a memset ends up clearing the new pll config. > > > > Hm, I forgot about this split totally. And there seems to be a giant mess > > going on here: > > > > In our top-level intel_atomic_check we have 4 parts to compute state: > > 1. drm_atomic_helper_check_modeset > > 2. intel_modeset_pipe_config > > 3. intel_modeset_checks > > 4. drm_atomic_helper_check_planes > > > > We recalculate clocks (by calling dev_priv->display.crtc_compute_clock) > > in 1., way ahead of anything else in intel_crtc_atomic_check. That looks > > very suspcious since it means only very later on (in the loop that does > > 2.) do we even decide whether we need to do a full modeset or not. > > > > So what I had in mind is that we clear clocks in > > intel_modeset_pipe_config, before we call any of the callbacks. That makes > > sure that when we decided to do a modeset, we do recompute the clocks > > correctly. > > I had a suspicion this would interact badly with how we "cancel" the modeset if the pipe config > didn't changed, just after the call to intel_modeset_pipe_config(). It turns out there's an issue > there already. > > There are two possibilities for the dpll_hw_state value after the new pipe_config is calculated. It > may have the new values already for DP in HSW/BDW and eDP in SKL or it may still have the old value. > In the latter case the new value is only calculated in .crtc_clock(), after we already compared the > old and new configs and may have decided to skip the modeset. > > But doing the memset() in intel_modeset_pipe_config() would be find as long as we don't change our > minds about doing a modeset later. It's more annoying since my analysis is all wrong: intel_crtc_atomic_check is called from drm_atomic_helper_check_planes, i.e. step 4 not step 1. It'll all work out I think if we memset it in intel_modeset_pipe_config. The caveat is that we need to move the clock recomputation into intel_modeset_pipe_config too (which is better, since then we'll have more accurate state to decided whether we'll fastboot or not). And then intel_modeset_clear_plls would really just update the global pll setup (and would be really good to rename it to intel_modeset_compute_shared_dpll or whatever). Thoughts? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-10-14 15:03 ` Daniel Vetter @ 2015-11-10 12:53 ` Jani Nikula 2015-11-11 9:25 ` Ander Conselvan De Oliveira 0 siblings, 1 reply; 19+ messages in thread From: Jani Nikula @ 2015-11-10 12:53 UTC (permalink / raw) To: Daniel Vetter, Ander Conselvan De Oliveira, Lankhorst, Maarten; +Cc: intel-gfx On Wed, 14 Oct 2015, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Oct 14, 2015 at 04:58:55PM +0300, Ander Conselvan De Oliveira wrote: >> On Wed, 2015-10-14 at 14:44 +0200, Daniel Vetter wrote: >> > On Wed, Oct 14, 2015 at 11:21:46AM +0300, Ander Conselvan De Oliveira wrote: >> > > On Tue, 2015-10-13 at 16:08 +0200, Daniel Vetter wrote: >> > > > On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote: >> > > > > Op 13-10-15 om 15:58 schreef Daniel Vetter: >> > > > > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst wrote: >> > > > > > > Op 13-10-15 om 15:35 schreef Daniel Vetter: >> > > > > > > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst wrote: >> > > > > > > > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: >> > > > > > > > > > Using 2 connectors (DVI and VGA) will cause wrpll to be set for >> > > > > > > > > > INTEL_OUTPUT_HDMI but never reset if switching to INTEL_OUTPUT_VGA >> > > > > > > > > > >> > > > > > > > > > Supresses errors like these: >> > > > > > > > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll >> > > > > > > > > > >> > > > > > > > > Looks like a good idea to always zero it. >> > > > > > > > Except that we still have a bunch of cases where we recompute clock state >> > > > > > > > but only partially. Can we just move them all up into a common place >> > > > > > > > please? That would also catch cases where we simply forget to fill this >> > > > > > > > out at all. >> > > > > > > > >> > > > > > > > One case I noticed is edp in skl_ddi_pll_select, but there's probably >> > > > > > > > more. >> > > > > > > > >> > > > > > > Something like below, with all the memsets for dpll_hw_state removed? >> > > > > > I think this will blow up since we recompute clock state only when >> > > > > > needs_modeset is true. So needs a bit more intelligence in deciding when >> > > > > > to clear it I think. >> > > > > Oops you're right. Maybe intel_modeset_clear_plls because that's where all the clock state >> > > > > belongs? >> > > > >> > > > Yeah that might be an even better place, in the loop after the continue; >> > > > statement. >> > > >> > > The reason I didn't put the memset there in the first place was the way we calculate plls for DP >> > > with DDI platforms. In that case, ddi_pll_sel is setup from the encoder_config instead of >> > > compute_clock, so a memset ends up clearing the new pll config. >> > >> > Hm, I forgot about this split totally. And there seems to be a giant mess >> > going on here: >> > >> > In our top-level intel_atomic_check we have 4 parts to compute state: >> > 1. drm_atomic_helper_check_modeset >> > 2. intel_modeset_pipe_config >> > 3. intel_modeset_checks >> > 4. drm_atomic_helper_check_planes >> > >> > We recalculate clocks (by calling dev_priv->display.crtc_compute_clock) >> > in 1., way ahead of anything else in intel_crtc_atomic_check. That looks >> > very suspcious since it means only very later on (in the loop that does >> > 2.) do we even decide whether we need to do a full modeset or not. >> > >> > So what I had in mind is that we clear clocks in >> > intel_modeset_pipe_config, before we call any of the callbacks. That makes >> > sure that when we decided to do a modeset, we do recompute the clocks >> > correctly. >> >> I had a suspicion this would interact badly with how we "cancel" the modeset if the pipe config >> didn't changed, just after the call to intel_modeset_pipe_config(). It turns out there's an issue >> there already. >> >> There are two possibilities for the dpll_hw_state value after the new pipe_config is calculated. It >> may have the new values already for DP in HSW/BDW and eDP in SKL or it may still have the old value. >> In the latter case the new value is only calculated in .crtc_clock(), after we already compared the >> old and new configs and may have decided to skip the modeset. >> >> But doing the memset() in intel_modeset_pipe_config() would be find as long as we don't change our >> minds about doing a modeset later. > > It's more annoying since my analysis is all wrong: intel_crtc_atomic_check > is called from drm_atomic_helper_check_planes, i.e. step 4 not step 1. > It'll all work out I think if we memset it in intel_modeset_pipe_config. > The caveat is that we need to move the clock recomputation into > intel_modeset_pipe_config too (which is better, since then we'll have more > accurate state to decided whether we'll fastboot or not). > > And then intel_modeset_clear_plls would really just update the global pll > setup (and would be really good to rename it to > intel_modeset_compute_shared_dpll or whatever). > > Thoughts? Ander, Maarten, where are we with this? Is it horribly wrong to merge the original patch in this ever-growing and diverging thread? BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-11-10 12:53 ` Jani Nikula @ 2015-11-11 9:25 ` Ander Conselvan De Oliveira 2015-11-11 14:21 ` Jani Nikula 0 siblings, 1 reply; 19+ messages in thread From: Ander Conselvan De Oliveira @ 2015-11-11 9:25 UTC (permalink / raw) To: Jani Nikula, Daniel Vetter, Lankhorst, Maarten; +Cc: intel-gfx On Tue, 2015-11-10 at 14:53 +0200, Jani Nikula wrote: > On Wed, 14 Oct 2015, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Oct 14, 2015 at 04:58:55PM +0300, Ander Conselvan De Oliveira wrote: > > > On Wed, 2015-10-14 at 14:44 +0200, Daniel Vetter wrote: > > > > On Wed, Oct 14, 2015 at 11:21:46AM +0300, Ander Conselvan De Oliveira > > > > wrote: > > > > > On Tue, 2015-10-13 at 16:08 +0200, Daniel Vetter wrote: > > > > > > On Tue, Oct 13, 2015 at 04:00:37PM +0200, Maarten Lankhorst wrote: > > > > > > > Op 13-10-15 om 15:58 schreef Daniel Vetter: > > > > > > > > On Tue, Oct 13, 2015 at 03:43:28PM +0200, Maarten Lankhorst > > > > > > > > wrote: > > > > > > > > > Op 13-10-15 om 15:35 schreef Daniel Vetter: > > > > > > > > > > On Tue, Oct 13, 2015 at 03:18:16PM +0200, Maarten Lankhorst > > > > > > > > > > wrote: > > > > > > > > > > > Op 23-09-15 om 17:34 schreef Gabriel Feceoru: > > > > > > > > > > > > Using 2 connectors (DVI and VGA) will cause wrpll to be > > > > > > > > > > > > set for > > > > > > > > > > > > INTEL_OUTPUT_HDMI but never reset if switching to > > > > > > > > > > > > INTEL_OUTPUT_VGA > > > > > > > > > > > > > > > > > > > > > > > > Supresses errors like these: > > > > > > > > > > > > [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch > > > > > > > > > > > > in dpll_hw_state.wrpll > > > > > > > > > > > > > > > > > > > > > > > Looks like a good idea to always zero it. > > > > > > > > > > Except that we still have a bunch of cases where we > > > > > > > > > > recompute clock state > > > > > > > > > > but only partially. Can we just move them all up into a > > > > > > > > > > common place > > > > > > > > > > please? That would also catch cases where we simply forget > > > > > > > > > > to fill this > > > > > > > > > > out at all. > > > > > > > > > > > > > > > > > > > > One case I noticed is edp in skl_ddi_pll_select, but there's > > > > > > > > > > probably > > > > > > > > > > more. > > > > > > > > > > > > > > > > > > > Something like below, with all the memsets for dpll_hw_state > > > > > > > > > removed? > > > > > > > > I think this will blow up since we recompute clock state only > > > > > > > > when > > > > > > > > needs_modeset is true. So needs a bit more intelligence in > > > > > > > > deciding when > > > > > > > > to clear it I think. > > > > > > > Oops you're right. Maybe intel_modeset_clear_plls because that's > > > > > > > where all the clock state > > > > > > > belongs? > > > > > > > > > > > > Yeah that might be an even better place, in the loop after the > > > > > > continue; > > > > > > statement. > > > > > > > > > > The reason I didn't put the memset there in the first place was the > > > > > way we calculate plls for DP > > > > > with DDI platforms. In that case, ddi_pll_sel is setup from the > > > > > encoder_config instead of > > > > > compute_clock, so a memset ends up clearing the new pll config. > > > > > > > > Hm, I forgot about this split totally. And there seems to be a giant > > > > mess > > > > going on here: > > > > > > > > In our top-level intel_atomic_check we have 4 parts to compute state: > > > > 1. drm_atomic_helper_check_modeset > > > > 2. intel_modeset_pipe_config > > > > 3. intel_modeset_checks > > > > 4. drm_atomic_helper_check_planes > > > > > > > > We recalculate clocks (by calling dev_priv->display.crtc_compute_clock) > > > > in 1., way ahead of anything else in intel_crtc_atomic_check. That looks > > > > very suspcious since it means only very later on (in the loop that does > > > > 2.) do we even decide whether we need to do a full modeset or not. > > > > > > > > So what I had in mind is that we clear clocks in > > > > intel_modeset_pipe_config, before we call any of the callbacks. That > > > > makes > > > > sure that when we decided to do a modeset, we do recompute the clocks > > > > correctly. > > > > > > I had a suspicion this would interact badly with how we "cancel" the > > > modeset if the pipe config > > > didn't changed, just after the call to intel_modeset_pipe_config(). It > > > turns out there's an issue > > > there already. > > > > > > There are two possibilities for the dpll_hw_state value after the new > > > pipe_config is calculated. It > > > may have the new values already for DP in HSW/BDW and eDP in SKL or it may > > > still have the old value. > > > In the latter case the new value is only calculated in .crtc_clock(), > > > after we already compared the > > > old and new configs and may have decided to skip the modeset. > > > > > > But doing the memset() in intel_modeset_pipe_config() would be find as > > > long as we don't change our > > > minds about doing a modeset later. > > > > It's more annoying since my analysis is all wrong: intel_crtc_atomic_check > > is called from drm_atomic_helper_check_planes, i.e. step 4 not step 1. > > It'll all work out I think if we memset it in intel_modeset_pipe_config. > > The caveat is that we need to move the clock recomputation into > > intel_modeset_pipe_config too (which is better, since then we'll have more > > accurate state to decided whether we'll fastboot or not). > > > > And then intel_modeset_clear_plls would really just update the global pll > > setup (and would be really good to rename it to > > intel_modeset_compute_shared_dpll or whatever). > > > > Thoughts? > > Ander, Maarten, where are we with this? Is it horribly wrong to merge > the original patch in this ever-growing and diverging thread? I think the patch as is will cause problems with DP, since we might clear the pll selection made in hsw_dp_set_ddi_pll_sel(). I think the easy fix disregarding the discussion in this thread is to drop another memset in intel_crt_compute_config(). Like this diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index b84aaa0..ad099f3 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -278,6 +278,9 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder, /* FDI must always be 2.7 GHz */ if (HAS_DDI(dev)) { + memset(&pipe_config->dpll_hw_state, 0, + sizeof(pipe_config->dpll_hw_state)); + pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL; pipe_config->port_clock = 135000 * 2; } Ander _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-11-11 9:25 ` Ander Conselvan De Oliveira @ 2015-11-11 14:21 ` Jani Nikula 2015-11-11 16:41 ` [PATCH] drm/i915: Clear DDI pll selection in intel_crtc_compute_config() Ander Conselvan de Oliveira 2015-11-11 18:27 ` [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw Gabriel Feceoru 0 siblings, 2 replies; 19+ messages in thread From: Jani Nikula @ 2015-11-11 14:21 UTC (permalink / raw) To: Ander Conselvan De Oliveira, Daniel Vetter, Lankhorst, Maarten; +Cc: intel-gfx On Wed, 11 Nov 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote: > On Tue, 2015-11-10 at 14:53 +0200, Jani Nikula wrote: >> Ander, Maarten, where are we with this? Is it horribly wrong to merge >> the original patch in this ever-growing and diverging thread? > > I think the patch as is will cause problems with DP, since we might clear the > pll selection made in hsw_dp_set_ddi_pll_sel(). I think the easy fix > disregarding the discussion in this thread is to drop another memset in > intel_crt_compute_config(). Like this Ander, please post this as a proper patch for review. BR, Jani. > > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > index b84aaa0..ad099f3 100644 > --- a/drivers/gpu/drm/i915/intel_crt.c > +++ b/drivers/gpu/drm/i915/intel_crt.c > @@ -278,6 +278,9 @@ static bool intel_crt_compute_config(struct intel_encoder > *encoder, > > /* FDI must always be 2.7 GHz */ > if (HAS_DDI(dev)) { > + memset(&pipe_config->dpll_hw_state, 0, > + sizeof(pipe_config->dpll_hw_state)); > + > pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL; > pipe_config->port_clock = 135000 * 2; > } > > Ander -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] drm/i915: Clear DDI pll selection in intel_crtc_compute_config() 2015-11-11 14:21 ` Jani Nikula @ 2015-11-11 16:41 ` Ander Conselvan de Oliveira 2015-11-11 18:27 ` [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw Gabriel Feceoru 1 sibling, 0 replies; 19+ messages in thread From: Ander Conselvan de Oliveira @ 2015-11-11 16:41 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira Clear the pipe's dpll_hw_state when choosing the PLL for CRT on DDI platforms. Otherwise stale values might cause the state checker to complain. Should fix errors like below: [drm:intel_pipe_config_compare [i915]] *ERROR* mismatch in dpll_hw_state.wrpll Cc: Gabriel Feceoru <gabriel.feceoru@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Jani Nikula <jani.nikula@linux.intel.com> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- *Completely untested* drivers/gpu/drm/i915/intel_crt.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index b84aaa0..ad099f3 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -278,6 +278,9 @@ static bool intel_crt_compute_config(struct intel_encoder *encoder, /* FDI must always be 2.7 GHz */ if (HAS_DDI(dev)) { + memset(&pipe_config->dpll_hw_state, 0, + sizeof(pipe_config->dpll_hw_state)); + pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL; pipe_config->port_clock = 135000 * 2; } -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-11-11 14:21 ` Jani Nikula 2015-11-11 16:41 ` [PATCH] drm/i915: Clear DDI pll selection in intel_crtc_compute_config() Ander Conselvan de Oliveira @ 2015-11-11 18:27 ` Gabriel Feceoru 2015-11-12 8:28 ` Lankhorst, Maarten 1 sibling, 1 reply; 19+ messages in thread From: Gabriel Feceoru @ 2015-11-11 18:27 UTC (permalink / raw) To: Jani Nikula, Ander Conselvan De Oliveira, Daniel Vetter, Lankhorst, Maarten Cc: intel-gfx On 11.11.2015 16:21, Jani Nikula wrote: > On Wed, 11 Nov 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote: >> On Tue, 2015-11-10 at 14:53 +0200, Jani Nikula wrote: >>> Ander, Maarten, where are we with this? Is it horribly wrong to merge >>> the original patch in this ever-growing and diverging thread? >> >> I think the patch as is will cause problems with DP, since we might clear the >> pll selection made in hsw_dp_set_ddi_pll_sel(). I think the easy fix >> disregarding the discussion in this thread is to drop another memset in >> intel_crt_compute_config(). Like this > > > Ander, please post this as a proper patch for review. > > BR, > Jani. Hi, I tested this patch on my system and I can confirm it fixes the original issue. However there are a few memset in *_ddi_pll_select functions which might not be needed anymore. For instance I tried to remove the hsw one and didn't see any regression. Regards, Gabriel > >> >> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c >> index b84aaa0..ad099f3 100644 >> --- a/drivers/gpu/drm/i915/intel_crt.c >> +++ b/drivers/gpu/drm/i915/intel_crt.c >> @@ -278,6 +278,9 @@ static bool intel_crt_compute_config(struct intel_encoder >> *encoder, >> >> /* FDI must always be 2.7 GHz */ >> if (HAS_DDI(dev)) { >> + memset(&pipe_config->dpll_hw_state, 0, >> + sizeof(pipe_config->dpll_hw_state)); >> + >> pipe_config->ddi_pll_sel = PORT_CLK_SEL_SPLL; >> pipe_config->port_clock = 135000 * 2; >> } >> >> Ander > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-11-11 18:27 ` [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw Gabriel Feceoru @ 2015-11-12 8:28 ` Lankhorst, Maarten 2015-11-12 18:35 ` Gabriel Feceoru 0 siblings, 1 reply; 19+ messages in thread From: Lankhorst, Maarten @ 2015-11-12 8:28 UTC (permalink / raw) To: Feceoru, Gabriel; +Cc: intel-gfx Hey, Gabriel Feceoru schreef op wo 11-11-2015 om 20:27 [+0200]: > > On 11.11.2015 16:21, Jani Nikula wrote: > > On Wed, 11 Nov 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote: > >> On Tue, 2015-11-10 at 14:53 +0200, Jani Nikula wrote: > >>> Ander, Maarten, where are we with this? Is it horribly wrong to merge > >>> the original patch in this ever-growing and diverging thread? > >> > >> I think the patch as is will cause problems with DP, since we might clear the > >> pll selection made in hsw_dp_set_ddi_pll_sel(). I think the easy fix > >> disregarding the discussion in this thread is to drop another memset in > >> intel_crt_compute_config(). Like this > > > > > > Ander, please post this as a proper patch for review. > > > > BR, > > Jani. > > Hi, > I tested this patch on my system and I can confirm it fixes the original > issue. > However there are a few memset in *_ddi_pll_select functions which might > not be needed anymore. For instance I tried to remove the hsw one and > didn't see any regression. Could you test http://lists.freedesktop.org/archives/intel-gfx/2015-September/075964.html ? Should be a less duct-tape fix.. ~Maarten --------------------------------------------------------------------- Intel International B.V. Registered in The Netherlands under number 34098535 Statutory seat: Haarlemmermeer Registered address: Capronilaan 37, 1119NG Schiphol-Rijk This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw 2015-11-12 8:28 ` Lankhorst, Maarten @ 2015-11-12 18:35 ` Gabriel Feceoru 0 siblings, 0 replies; 19+ messages in thread From: Gabriel Feceoru @ 2015-11-12 18:35 UTC (permalink / raw) To: Lankhorst, Maarten; +Cc: intel-gfx On 12.11.2015 10:28, Lankhorst, Maarten wrote: > Hey, > > > Gabriel Feceoru schreef op wo 11-11-2015 om 20:27 [+0200]: >> >> On 11.11.2015 16:21, Jani Nikula wrote: >>> On Wed, 11 Nov 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote: >>>> On Tue, 2015-11-10 at 14:53 +0200, Jani Nikula wrote: >>>>> Ander, Maarten, where are we with this? Is it horribly wrong to merge >>>>> the original patch in this ever-growing and diverging thread? >>>> >>>> I think the patch as is will cause problems with DP, since we might clear the >>>> pll selection made in hsw_dp_set_ddi_pll_sel(). I think the easy fix >>>> disregarding the discussion in this thread is to drop another memset in >>>> intel_crt_compute_config(). Like this >>> >>> >>> Ander, please post this as a proper patch for review. >>> >>> BR, >>> Jani. >> >> Hi, >> I tested this patch on my system and I can confirm it fixes the original >> issue. >> However there are a few memset in *_ddi_pll_select functions which might >> not be needed anymore. For instance I tried to remove the hsw one and >> didn't see any regression. > > Could you test > http://lists.freedesktop.org/archives/intel-gfx/2015-September/075964.html > ? > > Should be a less duct-tape fix.. Hi Marteen, I tested this (hsw only) and this is a good fix, too. I get similar results with Ander's patch. Gabriel. > > ~Maarten > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-11-12 18:28 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-09-23 15:34 [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw Gabriel Feceoru 2015-10-13 13:18 ` Maarten Lankhorst 2015-10-13 13:35 ` Daniel Vetter 2015-10-13 13:43 ` Daniel Vetter 2015-10-13 13:43 ` Maarten Lankhorst 2015-10-13 13:58 ` Daniel Vetter 2015-10-13 14:00 ` Maarten Lankhorst 2015-10-13 14:08 ` Daniel Vetter 2015-10-14 8:21 ` Ander Conselvan De Oliveira 2015-10-14 12:44 ` Daniel Vetter 2015-10-14 13:58 ` Ander Conselvan De Oliveira 2015-10-14 15:03 ` Daniel Vetter 2015-11-10 12:53 ` Jani Nikula 2015-11-11 9:25 ` Ander Conselvan De Oliveira 2015-11-11 14:21 ` Jani Nikula 2015-11-11 16:41 ` [PATCH] drm/i915: Clear DDI pll selection in intel_crtc_compute_config() Ander Conselvan de Oliveira 2015-11-11 18:27 ` [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw Gabriel Feceoru 2015-11-12 8:28 ` Lankhorst, Maarten 2015-11-12 18:35 ` Gabriel Feceoru
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.