| Message ID | 20180427231208.9092-1-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <arthur.j.runyan@intel.com> > Cc: James Ausmus <james.ausmus@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > 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, > &intel_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
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 <arthur.j.runyan@intel.com> > > Cc: James Ausmus <james.ausmus@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > 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, > > &intel_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 > >
Em Qua, 2018-05-02 às 22:23 +0000, Patchwork escreveu: > == Series Details == > > Series: drm/i915: configure the transcoder clocks before touching > pipeconf on HSW+ (rev2) > URL : https://patchwork.freedesktop.org/series/42436/ > State : failure > > == Summary == > > = CI Bug Log - changes from CI_DRM_4125 -> Patchwork_8884 = > > == Summary - FAILURE == > > Serious unknown changes coming with Patchwork_8884 absolutely need > to be > verified manually. > > If you think the reported changes have nothing to do with the > changes > introduced in Patchwork_8884, please notify your bug team Can't the bot just give me the emails to cc? > to allow them > to document this new failure mode, which will reduce false > positives in CI. There were 2 runs for this series, both with different failures. The FBC one is caused by a FIFO underrun (which looks like we never properly fixed, and FBC is disabled on HSW anyway). In the other email there's a possible PNV regresison which definitely has nothing to do with this series. The other is a wedged GPU on module reload, which also doesn't seem to be caused by us. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/4243 > 6/revisions/2/mbox/ > > == Possible new issues == > > Here are the unknown changes that may have been introduced in > Patchwork_8884: > > === IGT changes === > > ==== Possible regressions ==== > > igt@kms_frontbuffer_tracking@basic: > fi-hsw-peppy: PASS -> DMESG-FAIL > > > == Known issues == > > Here are the changes found in Patchwork_8884 that come from known > issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@gem_mmap_gtt@basic-small-bo-tiledx: > fi-gdg-551: PASS -> FAIL (fdo#102575) > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c: > fi-bxt-dsi: PASS -> INCOMPLETE (fdo#103927) > > > ==== Possible fixes ==== > > igt@kms_flip@basic-flip-vs-wf_vblank: > fi-hsw-4200u: FAIL (fdo#100368) -> PASS > > igt@kms_frontbuffer_tracking@basic: > fi-hsw-4200u: DMESG-FAIL (fdo#106103, fdo#102614) -> PASS > > igt@kms_pipe_crc_basic@hang-read-crc-pipe-b: > fi-skl-6700k2: FAIL (fdo#103191) -> PASS > > > fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 > fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575 > fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614 > fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191 > fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927 > fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103 > > > == Participating hosts (39 -> 37) == > > Additional (1): fi-byt-j1900 > Missing (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq > > > == Build changes == > > * Linux: CI_DRM_4125 -> Patchwork_8884 > > CI_DRM_4125: c862a73270499cdd3d7c04c889e43303ae5f928e @ > git://anongit.freedesktop.org/gfx-ci/linux > IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ > git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_8884: 4a5b649073070534c7a0f7a1b828b2c3f7acd690 @ > git://anongit.freedesktop.org/gfx-ci/linux > piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ > git://anongit.freedesktop.org/piglit > > > == Linux commits == > > 4a5b64907307 drm/i915: enable the pipe/transcoder/planes later on > HSW+ > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchw > ork_8884/issues.html
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, &intel_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;
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 <arthur.j.runyan@intel.com> Cc: James Ausmus <james.ausmus@intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- 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.