Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Lankhorst, Maarten]

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  
> > 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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Gabriel Feceoru]

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  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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Ander Conselvan De Oliveira]

On Tue, 2015-11-10 at 14:53 +0200, Jani Nikula wrote:
> On Wed, 14 Oct 2015, Daniel Vetter  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 

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Jani Nikula]

On Wed, 11 Nov 2015, Ander Conselvan De Oliveira  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(_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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Jani Nikula]

On Wed, 14 Oct 2015, Daniel Vetter  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 

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Daniel Vetter]

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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Ander Conselvan De Oliveira]

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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Daniel Vetter]

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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Ander Conselvan De Oliveira]

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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[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.
-Daniel
> 
> Reviewed-by: Maarten Lankhorst 
> ___
> 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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[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.
-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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Daniel Vetter]

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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Maarten Lankhorst]

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 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Maarten Lankhorst]

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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Maarten Lankhorst]

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

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Daniel Vetter]

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

[Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[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

Signed-off-by: Gabriel Feceoru 
---
 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(_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(_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