Re: [Intel-gfx] [PATCH] drm/i915: configure the transcoder clocks before touching pipeconf on HSW+

[Paulo Zanoni]

Em Seg, 2018-04-30 às 21:12 +0300, Ville Syrjälä escreveu:
> On Fri, Apr 27, 2018 at 04:12:08PM -0700, Paulo Zanoni wrote:
> > For all platforms that run haswell_crtc_enable, our spec tells us
> > to
> > configure the transcoder clocks before it tells us to set pipeconf
> > and
> > the other pipe/transcoder/plane registers.
> > 
> > For some reason we've been able to get away with doing what we were
> > doing until now, but starting from Icelake, we get machine hangs if
> > we
> > try to touch the pipe/transcoder registers without having the
> > clocks
> > configured.
> > 
> > So this patch changes all the relevant platforms to call
> > intel_ddi_enable_pipe_clock at the point we're supposed to,
> > according
> > to the spec.
> 
> I don't think this really matches the spec. You're now enabling the
> clock before configuring the port stuff and link training. So AFAICS
> intel_ddi_enable_pipe_clock() seems to be in the correct place, but
> we're just configuring all the pipe/transcoder/etc. stuff way too
> early.

You're right, I missed the link training part, it needs to come before.

But anyway, moving enable_pipe_clock() + link_training ()up is the same
as moving all the pipe/transcoder/etc stuff down, the only difference
being when we set intel_crtc->active. I'll write a second version.

> 
> > 
> > It seems there is a way to work around this problem on ICL with
> > some
> > chicken bit, but I still couldn't find which one it is, and it's
> > better if we just do the right thing here.

We got the info we wanted and now know how to work around the problem,
but I still think we should go for the solution where we program the
hardware in the order it expects to be programmed.

Or we could do both: program correctly *and* enable the chicken bits.

Thanks,
Paulo

> > 
> > Cc: Arthur J Runyan 
> > Cc: James Ausmus 
> > Signed-off-by: Paulo Zanoni 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > Luckily our CI system should be in a spot where it is able to tell
> > us
> > whether this patch is good with high confidence. I haven't tested
> > it
> > on every affected platform.
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 48576ea2d36c..c93aed2ec16d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5578,8 +5578,11 @@ static void haswell_crtc_enable(struct
> > intel_crtc_state *pipe_config,
> >  _crtc->config->fdi_m_n, 
> > NULL);
> > }
> >  
> > -   if (!transcoder_is_dsi(cpu_transcoder))
> > +   if (!transcoder_is_dsi(cpu_transcoder)) {
> > +   intel_ddi_enable_pipe_clock(pipe_config);
> > +
> > haswell_set_pipeconf(crtc);
> > +   }
> >  
> > haswell_set_pipemisc(crtc);
> >  
> > @@ -5589,9 +5592,6 @@ static void haswell_crtc_enable(struct
> > intel_crtc_state *pipe_config,
> >  
> > intel_encoders_pre_enable(crtc, pipe_config, old_state);
> >  
> > -   if (!transcoder_is_dsi(cpu_transcoder))
> > -   intel_ddi_enable_pipe_clock(pipe_config);
> > -
> > /* Display WA #1180: WaDisableScalarClockGating: glk, cnl
> > */
> > psl_clkgate_wa = (IS_GEMINILAKE(dev_priv) ||
> > IS_CANNONLAKE(dev_priv)) &&
> >  intel_crtc->config->pch_pfit.enabled;
> > -- 
> > 2.14.3
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: configure the transcoder clocks before touching pipeconf on HSW+

[Ville Syrjälä]

On Fri, Apr 27, 2018 at 04:12:08PM -0700, Paulo Zanoni wrote:
> For all platforms that run haswell_crtc_enable, our spec tells us to
> configure the transcoder clocks before it tells us to set pipeconf and
> the other pipe/transcoder/plane registers.
> 
> For some reason we've been able to get away with doing what we were
> doing until now, but starting from Icelake, we get machine hangs if we
> try to touch the pipe/transcoder registers without having the clocks
> configured.
> 
> So this patch changes all the relevant platforms to call
> intel_ddi_enable_pipe_clock at the point we're supposed to, according
> to the spec.

I don't think this really matches the spec. You're now enabling the
clock before configuring the port stuff and link training. So AFAICS
intel_ddi_enable_pipe_clock() seems to be in the correct place, but
we're just configuring all the pipe/transcoder/etc. stuff way too
early.

> 
> It seems there is a way to work around this problem on ICL with some
> chicken bit, but I still couldn't find which one it is, and it's
> better if we just do the right thing here.
> 
> Cc: Arthur J Runyan 
> Cc: James Ausmus 
> Signed-off-by: Paulo Zanoni 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Luckily our CI system should be in a spot where it is able to tell us
> whether this patch is good with high confidence. I haven't tested it
> on every affected platform.
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 48576ea2d36c..c93aed2ec16d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5578,8 +5578,11 @@ static void haswell_crtc_enable(struct 
> intel_crtc_state *pipe_config,
>_crtc->config->fdi_m_n, NULL);
>   }
>  
> - if (!transcoder_is_dsi(cpu_transcoder))
> + if (!transcoder_is_dsi(cpu_transcoder)) {
> + intel_ddi_enable_pipe_clock(pipe_config);
> +
>   haswell_set_pipeconf(crtc);
> + }
>  
>   haswell_set_pipemisc(crtc);
>  
> @@ -5589,9 +5592,6 @@ static void haswell_crtc_enable(struct intel_crtc_state 
> *pipe_config,
>  
>   intel_encoders_pre_enable(crtc, pipe_config, old_state);
>  
> - if (!transcoder_is_dsi(cpu_transcoder))
> - intel_ddi_enable_pipe_clock(pipe_config);
> -
>   /* Display WA #1180: WaDisableScalarClockGating: glk, cnl */
>   psl_clkgate_wa = (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) &&
>intel_crtc->config->pch_pfit.enabled;
> -- 
> 2.14.3
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: configure the transcoder clocks before touching pipeconf on HSW+

[Paulo Zanoni]

For all platforms that run haswell_crtc_enable, our spec tells us to
configure the transcoder clocks before it tells us to set pipeconf and
the other pipe/transcoder/plane registers.

For some reason we've been able to get away with doing what we were
doing until now, but starting from Icelake, we get machine hangs if we
try to touch the pipe/transcoder registers without having the clocks
configured.

So this patch changes all the relevant platforms to call
intel_ddi_enable_pipe_clock at the point we're supposed to, according
to the spec.

It seems there is a way to work around this problem on ICL with some
chicken bit, but I still couldn't find which one it is, and it's
better if we just do the right thing here.

Cc: Arthur J Runyan 
Cc: James Ausmus 
Signed-off-by: Paulo Zanoni 
---
 drivers/gpu/drm/i915/intel_display.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

Luckily our CI system should be in a spot where it is able to tell us
whether this patch is good with high confidence. I haven't tested it
on every affected platform.

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 48576ea2d36c..c93aed2ec16d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5578,8 +5578,11 @@ static void haswell_crtc_enable(struct intel_crtc_state 
*pipe_config,
 _crtc->config->fdi_m_n, NULL);
}
 
-   if (!transcoder_is_dsi(cpu_transcoder))
+   if (!transcoder_is_dsi(cpu_transcoder)) {
+   intel_ddi_enable_pipe_clock(pipe_config);
+
haswell_set_pipeconf(crtc);
+   }
 
haswell_set_pipemisc(crtc);
 
@@ -5589,9 +5592,6 @@ static void haswell_crtc_enable(struct intel_crtc_state 
*pipe_config,
 
intel_encoders_pre_enable(crtc, pipe_config, old_state);
 
-   if (!transcoder_is_dsi(cpu_transcoder))
-   intel_ddi_enable_pipe_clock(pipe_config);
-
/* Display WA #1180: WaDisableScalarClockGating: glk, cnl */
psl_clkgate_wa = (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) &&
 intel_crtc->config->pch_pfit.enabled;
-- 
2.14.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx