Re: [Intel-gfx] [PATCH] drm/i915: Clear pipe's pll hw state in hsw_dp_set_ddi_pll_sel()

2015-07-01 Thread Daniel Vetter
On Wed, Jul 01, 2015 at 05:54:06PM +0300, Ander Conselvan De Oliveira wrote:
> On Tue, 2015-06-30 at 18:41 +0300, Jani Nikula wrote:
> > On Tue, 30 Jun 2015, Daniel Vetter  wrote:
> > > On Tue, Jun 30, 2015 at 04:47:06PM +0300, Jani Nikula wrote:
> > >> On Tue, 30 Jun 2015, Ander Conselvan de Oliveira 
> > >>  wrote:
> > >> > Similarly to what is done for SKL, clear the dpll_hw_state of the pipe
> > >> > config in hsw_dp_set_ddi_pll_sel(), since it main contain stale values.
> > >> > That can happen if a crtc that was previously driving an HDMI connector
> > >> > switches to a DP connector. In that case, the wrpll field was left with
> > >> > its old value, leading to warnings like the one below:
> > >> >
> > >> > [drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.wrpll 
> > >> > (expected 0xb035061f, found 0x)
> > >> > [ cut here ]
> > >> > WARNING: CPU: 1 PID: 767 at drivers/gpu/drm/i915/intel_display.c:12324 
> > >> > check_crtc_state+0x975/0x10b0 [i915]()
> > >> > pipe state doesn't match!
> > >> >
> > >> > This regression was indroduced in
> > >> >
> > >> > commit dd3cd74acf12723045a64f1f2c6298ac7b34a5d5
> > >> > Author: Ander Conselvan de Oliveira 
> > >> > 
> > >> > Date:   Fri May 15 13:34:29 2015 +0300
> > >> >
> > >> > drm/i915: Don't overwrite (e)DP PLL selection on SKL
> > >> >
> > >> > Signed-off-by: Ander Conselvan de Oliveira 
> > >> > 
> > >> 
> > >> Reported-by: Linus Torvalds 
> > >> Tested-by: Jani Nikula 
> > >
> > > Yeah makes sense as a fix for 4.2. But for 4.3 I wonder whether the
> > > original commit that started this chain needs to be changed a bit:
> > >
> > > commit 4978cc93d9ac240b435ce60431aef24239b4c270
> > > Author: Ander Conselvan de Oliveira 
> > > 
> > > Date:   Tue Apr 21 17:13:21 2015 +0300
> > >
> > > drm/i915: Preserve shared DPLL information in new pipe_config
> > >
> > > All the trouble this caused is because it not only preserves the sharing
> > > config (in crtc_state->shared_dpll) but also the ->dpll_hw_state. And I
> > > think with Maarten's latest code (for 4.3) we'd just do an unconditional
> > > compute_config (need it for fast pfit updates and fastboot), which means
> > > the bogus values in ->dpll_hw_state aren't a problem any more since we'll
> > > overwrite them again. And then we could remove that sprinkle of memsets we
> > > have all over, which would be good (since the current approach is
> > > obviously a bit fragile). Anyway:
> > >
> > > Reviewed-by: Daniel Vetter 
> > 
> > Pushed to drm-intel-next-fixes, thanks for the patch and review. One
> > down, another one left to fix.
> 
> I made some progress on the second issue, but I'm afraid Jani might have
> a found a third bug. The warning he gets happens because we try to wait
> for vblanks while updating the primary plane during the modeset. At that
> point, the crtc is off. The problem is in intel_check_primary_plane(),
> which is called from drm_atomic_helper_check_planes(). That function
> makes decisions about waiting for a vblank based on intel_crtc->active.
> Since the check is called before we disable the crtcs, active might be
> true, even though the plane update is done with crtcs disable.
> 
> The patch below makes the warning go away, but I still need to figure
> out how to set crtc_state->planes_changed properly if we are going down
> that route.
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index dcb1d25..f14727c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12480,10 +12480,6 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  
> intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]");
>  
> -   ret = drm_atomic_helper_check_planes(state->dev, state);
> -   if (ret)
> -   return ERR_PTR(ret);
> -
> return pipe_config;
>  }
>  
> 
> The backtrace on Linus' machine is different, though. It comes from the
> call to intel_crtc_disable_planes() in __intel_set_mode(). That would
> indicate we have a crtc with crtc->state->enable == true but that is
> actually inactive. I'm still not sure how we can get in that state.

Using intel_crtc->active to precompute any kind of decisions won't work. I
guess we just need to delay the decision whether to make a vblank wait or
not to where we do the vblank wait, and use the (then current
intel_crtc->active) there. This will be fixed properly in 4.3.

I suspect Linus' backtrace is something similar - we try to precompute
what needs to be updated, get it wrong and the go boom. Sprinkling an

if (!intel_crtc->active)
return;

early return into the set_mode should help. But I haven't looked at what
4.2 looks precisely yet in this area - too much flux because of the atomic
conversion.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscri

Re: [Intel-gfx] [PATCH] drm/i915: Clear pipe's pll hw state in hsw_dp_set_ddi_pll_sel()

2015-07-01 Thread Ander Conselvan De Oliveira
On Tue, 2015-06-30 at 18:41 +0300, Jani Nikula wrote:
> On Tue, 30 Jun 2015, Daniel Vetter  wrote:
> > On Tue, Jun 30, 2015 at 04:47:06PM +0300, Jani Nikula wrote:
> >> On Tue, 30 Jun 2015, Ander Conselvan de Oliveira 
> >>  wrote:
> >> > Similarly to what is done for SKL, clear the dpll_hw_state of the pipe
> >> > config in hsw_dp_set_ddi_pll_sel(), since it main contain stale values.
> >> > That can happen if a crtc that was previously driving an HDMI connector
> >> > switches to a DP connector. In that case, the wrpll field was left with
> >> > its old value, leading to warnings like the one below:
> >> >
> >> > [drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.wrpll 
> >> > (expected 0xb035061f, found 0x)
> >> > [ cut here ]
> >> > WARNING: CPU: 1 PID: 767 at drivers/gpu/drm/i915/intel_display.c:12324 
> >> > check_crtc_state+0x975/0x10b0 [i915]()
> >> > pipe state doesn't match!
> >> >
> >> > This regression was indroduced in
> >> >
> >> > commit dd3cd74acf12723045a64f1f2c6298ac7b34a5d5
> >> > Author: Ander Conselvan de Oliveira 
> >> > 
> >> > Date:   Fri May 15 13:34:29 2015 +0300
> >> >
> >> > drm/i915: Don't overwrite (e)DP PLL selection on SKL
> >> >
> >> > Signed-off-by: Ander Conselvan de Oliveira 
> >> > 
> >> 
> >> Reported-by: Linus Torvalds 
> >> Tested-by: Jani Nikula 
> >
> > Yeah makes sense as a fix for 4.2. But for 4.3 I wonder whether the
> > original commit that started this chain needs to be changed a bit:
> >
> > commit 4978cc93d9ac240b435ce60431aef24239b4c270
> > Author: Ander Conselvan de Oliveira 
> > Date:   Tue Apr 21 17:13:21 2015 +0300
> >
> > drm/i915: Preserve shared DPLL information in new pipe_config
> >
> > All the trouble this caused is because it not only preserves the sharing
> > config (in crtc_state->shared_dpll) but also the ->dpll_hw_state. And I
> > think with Maarten's latest code (for 4.3) we'd just do an unconditional
> > compute_config (need it for fast pfit updates and fastboot), which means
> > the bogus values in ->dpll_hw_state aren't a problem any more since we'll
> > overwrite them again. And then we could remove that sprinkle of memsets we
> > have all over, which would be good (since the current approach is
> > obviously a bit fragile). Anyway:
> >
> > Reviewed-by: Daniel Vetter 
> 
> Pushed to drm-intel-next-fixes, thanks for the patch and review. One
> down, another one left to fix.

I made some progress on the second issue, but I'm afraid Jani might have
a found a third bug. The warning he gets happens because we try to wait
for vblanks while updating the primary plane during the modeset. At that
point, the crtc is off. The problem is in intel_check_primary_plane(),
which is called from drm_atomic_helper_check_planes(). That function
makes decisions about waiting for a vblank based on intel_crtc->active.
Since the check is called before we disable the crtcs, active might be
true, even though the plane update is done with crtcs disable.

The patch below makes the warning go away, but I still need to figure
out how to set crtc_state->planes_changed properly if we are going down
that route.

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index dcb1d25..f14727c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12480,10 +12480,6 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 
intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]");
 
-   ret = drm_atomic_helper_check_planes(state->dev, state);
-   if (ret)
-   return ERR_PTR(ret);
-
return pipe_config;
 }
 

The backtrace on Linus' machine is different, though. It comes from the
call to intel_crtc_disable_planes() in __intel_set_mode(). That would
indicate we have a crtc with crtc->state->enable == true but that is
actually inactive. I'm still not sure how we can get in that state.

Ander

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [Intel-gfx] [PATCH] drm/i915: Clear pipe's pll hw state in hsw_dp_set_ddi_pll_sel()

2015-06-30 Thread Jani Nikula
On Tue, 30 Jun 2015, Daniel Vetter  wrote:
> On Tue, Jun 30, 2015 at 04:47:06PM +0300, Jani Nikula wrote:
>> On Tue, 30 Jun 2015, Ander Conselvan de Oliveira 
>>  wrote:
>> > Similarly to what is done for SKL, clear the dpll_hw_state of the pipe
>> > config in hsw_dp_set_ddi_pll_sel(), since it main contain stale values.
>> > That can happen if a crtc that was previously driving an HDMI connector
>> > switches to a DP connector. In that case, the wrpll field was left with
>> > its old value, leading to warnings like the one below:
>> >
>> > [drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.wrpll 
>> > (expected 0xb035061f, found 0x)
>> > [ cut here ]
>> > WARNING: CPU: 1 PID: 767 at drivers/gpu/drm/i915/intel_display.c:12324 
>> > check_crtc_state+0x975/0x10b0 [i915]()
>> > pipe state doesn't match!
>> >
>> > This regression was indroduced in
>> >
>> > commit dd3cd74acf12723045a64f1f2c6298ac7b34a5d5
>> > Author: Ander Conselvan de Oliveira 
>> > Date:   Fri May 15 13:34:29 2015 +0300
>> >
>> > drm/i915: Don't overwrite (e)DP PLL selection on SKL
>> >
>> > Signed-off-by: Ander Conselvan de Oliveira 
>> > 
>> 
>> Reported-by: Linus Torvalds 
>> Tested-by: Jani Nikula 
>
> Yeah makes sense as a fix for 4.2. But for 4.3 I wonder whether the
> original commit that started this chain needs to be changed a bit:
>
> commit 4978cc93d9ac240b435ce60431aef24239b4c270
> Author: Ander Conselvan de Oliveira 
> Date:   Tue Apr 21 17:13:21 2015 +0300
>
> drm/i915: Preserve shared DPLL information in new pipe_config
>
> All the trouble this caused is because it not only preserves the sharing
> config (in crtc_state->shared_dpll) but also the ->dpll_hw_state. And I
> think with Maarten's latest code (for 4.3) we'd just do an unconditional
> compute_config (need it for fast pfit updates and fastboot), which means
> the bogus values in ->dpll_hw_state aren't a problem any more since we'll
> overwrite them again. And then we could remove that sprinkle of memsets we
> have all over, which would be good (since the current approach is
> obviously a bit fragile). Anyway:
>
> Reviewed-by: Daniel Vetter 

Pushed to drm-intel-next-fixes, thanks for the patch and review. One
down, another one left to fix.

BR,
Jani.


>
> Thanks, Daniel
>
>> 
>> > ---
>> >
>> > Only compile tested, for the wrpll warning.
>> >
>> > Thanks,
>> > Ander
>> >
>> >  drivers/gpu/drm/i915/intel_dp.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
>> > b/drivers/gpu/drm/i915/intel_dp.c
>> > index 4ebfc3a..fbd9ac3 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -1147,6 +1147,9 @@ skl_edp_set_pll_config(struct intel_crtc_state 
>> > *pipe_config, int link_clock)
>> >  static void
>> >  hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw)
>> >  {
>> > +  memset(&pipe_config->dpll_hw_state, 0,
>> > + sizeof(pipe_config->dpll_hw_state));
>> > +
>> >switch (link_bw) {
>> >case DP_LINK_BW_1_62:
>> >pipe_config->ddi_pll_sel = PORT_CLK_SEL_LCPLL_810;
>> > -- 
>> > 2.1.0
>> >
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>> ___
>> Intel-gfx mailing list
>> intel-...@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [Intel-gfx] [PATCH] drm/i915: Clear pipe's pll hw state in hsw_dp_set_ddi_pll_sel()

2015-06-30 Thread Daniel Vetter
On Tue, Jun 30, 2015 at 04:47:06PM +0300, Jani Nikula wrote:
> On Tue, 30 Jun 2015, Ander Conselvan de Oliveira 
>  wrote:
> > Similarly to what is done for SKL, clear the dpll_hw_state of the pipe
> > config in hsw_dp_set_ddi_pll_sel(), since it main contain stale values.
> > That can happen if a crtc that was previously driving an HDMI connector
> > switches to a DP connector. In that case, the wrpll field was left with
> > its old value, leading to warnings like the one below:
> >
> > [drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.wrpll 
> > (expected 0xb035061f, found 0x)
> > [ cut here ]
> > WARNING: CPU: 1 PID: 767 at drivers/gpu/drm/i915/intel_display.c:12324 
> > check_crtc_state+0x975/0x10b0 [i915]()
> > pipe state doesn't match!
> >
> > This regression was indroduced in
> >
> > commit dd3cd74acf12723045a64f1f2c6298ac7b34a5d5
> > Author: Ander Conselvan de Oliveira 
> > Date:   Fri May 15 13:34:29 2015 +0300
> >
> > drm/i915: Don't overwrite (e)DP PLL selection on SKL
> >
> > Signed-off-by: Ander Conselvan de Oliveira 
> > 
> 
> Reported-by: Linus Torvalds 
> Tested-by: Jani Nikula 

Yeah makes sense as a fix for 4.2. But for 4.3 I wonder whether the
original commit that started this chain needs to be changed a bit:

commit 4978cc93d9ac240b435ce60431aef24239b4c270
Author: Ander Conselvan de Oliveira 
Date:   Tue Apr 21 17:13:21 2015 +0300

drm/i915: Preserve shared DPLL information in new pipe_config

All the trouble this caused is because it not only preserves the sharing
config (in crtc_state->shared_dpll) but also the ->dpll_hw_state. And I
think with Maarten's latest code (for 4.3) we'd just do an unconditional
compute_config (need it for fast pfit updates and fastboot), which means
the bogus values in ->dpll_hw_state aren't a problem any more since we'll
overwrite them again. And then we could remove that sprinkle of memsets we
have all over, which would be good (since the current approach is
obviously a bit fragile). Anyway:

Reviewed-by: Daniel Vetter 

Thanks, Daniel

> 
> > ---
> >
> > Only compile tested, for the wrpll warning.
> >
> > Thanks,
> > Ander
> >
> >  drivers/gpu/drm/i915/intel_dp.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 4ebfc3a..fbd9ac3 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1147,6 +1147,9 @@ skl_edp_set_pll_config(struct intel_crtc_state 
> > *pipe_config, int link_clock)
> >  static void
> >  hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw)
> >  {
> > +   memset(&pipe_config->dpll_hw_state, 0,
> > +  sizeof(pipe_config->dpll_hw_state));
> > +
> > switch (link_bw) {
> > case DP_LINK_BW_1_62:
> > pipe_config->ddi_pll_sel = PORT_CLK_SEL_LCPLL_810;
> > -- 
> > 2.1.0
> >
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] drm/i915: Clear pipe's pll hw state in hsw_dp_set_ddi_pll_sel()

2015-06-30 Thread Jani Nikula
On Tue, 30 Jun 2015, Ander Conselvan de Oliveira 
 wrote:
> Similarly to what is done for SKL, clear the dpll_hw_state of the pipe
> config in hsw_dp_set_ddi_pll_sel(), since it main contain stale values.
> That can happen if a crtc that was previously driving an HDMI connector
> switches to a DP connector. In that case, the wrpll field was left with
> its old value, leading to warnings like the one below:
>
> [drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.wrpll 
> (expected 0xb035061f, found 0x)
> [ cut here ]
> WARNING: CPU: 1 PID: 767 at drivers/gpu/drm/i915/intel_display.c:12324 
> check_crtc_state+0x975/0x10b0 [i915]()
> pipe state doesn't match!
>
> This regression was indroduced in
>
> commit dd3cd74acf12723045a64f1f2c6298ac7b34a5d5
> Author: Ander Conselvan de Oliveira 
> Date:   Fri May 15 13:34:29 2015 +0300
>
> drm/i915: Don't overwrite (e)DP PLL selection on SKL
>
> Signed-off-by: Ander Conselvan de Oliveira 
> 

Reported-by: Linus Torvalds 
Tested-by: Jani Nikula 

> ---
>
> Only compile tested, for the wrpll warning.
>
> Thanks,
> Ander
>
>  drivers/gpu/drm/i915/intel_dp.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4ebfc3a..fbd9ac3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1147,6 +1147,9 @@ skl_edp_set_pll_config(struct intel_crtc_state 
> *pipe_config, int link_clock)
>  static void
>  hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw)
>  {
> + memset(&pipe_config->dpll_hw_state, 0,
> +sizeof(pipe_config->dpll_hw_state));
> +
>   switch (link_bw) {
>   case DP_LINK_BW_1_62:
>   pipe_config->ddi_pll_sel = PORT_CLK_SEL_LCPLL_810;
> -- 
> 2.1.0
>

-- 
Jani Nikula, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] drm/i915: Clear pipe's pll hw state in hsw_dp_set_ddi_pll_sel()

2015-06-30 Thread Ander Conselvan de Oliveira
Similarly to what is done for SKL, clear the dpll_hw_state of the pipe
config in hsw_dp_set_ddi_pll_sel(), since it main contain stale values.
That can happen if a crtc that was previously driving an HDMI connector
switches to a DP connector. In that case, the wrpll field was left with
its old value, leading to warnings like the one below:

[drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.wrpll (expected 
0xb035061f, found 0x)
[ cut here ]
WARNING: CPU: 1 PID: 767 at drivers/gpu/drm/i915/intel_display.c:12324 
check_crtc_state+0x975/0x10b0 [i915]()
pipe state doesn't match!

This regression was indroduced in

commit dd3cd74acf12723045a64f1f2c6298ac7b34a5d5
Author: Ander Conselvan de Oliveira 
Date:   Fri May 15 13:34:29 2015 +0300

drm/i915: Don't overwrite (e)DP PLL selection on SKL

Signed-off-by: Ander Conselvan de Oliveira 

---

Only compile tested, for the wrpll warning.

Thanks,
Ander

 drivers/gpu/drm/i915/intel_dp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4ebfc3a..fbd9ac3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1147,6 +1147,9 @@ skl_edp_set_pll_config(struct intel_crtc_state 
*pipe_config, int link_clock)
 static void
 hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw)
 {
+   memset(&pipe_config->dpll_hw_state, 0,
+  sizeof(pipe_config->dpll_hw_state));
+
switch (link_bw) {
case DP_LINK_BW_1_62:
pipe_config->ddi_pll_sel = PORT_CLK_SEL_LCPLL_810;
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/