| Message ID | 561D0A80.7050503@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > >
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?
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
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
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
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
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
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.
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;