| Message ID | 1416332752-28234-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=290/290->290/290
PNV: pass/total=362/362->362/362
ILK: pass/total=381/381->381/381
IVB: pass/total=522/559->522/559
SNB: pass/total=444/444->444/444
HSW: pass/total=595/595->595/595
BDW: pass/total=436/436->435/436
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
BDW: Intel_gpu_tools, igt_gem_reset_stats_close-pending-ctx-render, PASS(4, M28) -> FAIL(1, M28)PASS(3, M28)
On Tue, Nov 18, 2014 at 04:00:58PM -0800, shuang.he@intel.com wrote: > Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) > -------------------------------------Summary------------------------------------- > Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate > BYT: pass/total=290/290->290/290 > PNV: pass/total=362/362->362/362 > ILK: pass/total=381/381->381/381 > IVB: pass/total=522/559->522/559 > SNB: pass/total=444/444->444/444 > HSW: pass/total=595/595->595/595 > BDW: pass/total=436/436->435/436 > -------------------------------------Detailed------------------------------------- > test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)... > BDW: Intel_gpu_tools, igt_gem_reset_stats_close-pending-ctx-render, PASS(4, M28) -> FAIL(1, M28)PASS(3, M28) This patch /should/ fix a pretty decent amount of WARN backtrace in modeset tests on hsw/bdw (as long as there's a hdmi screen around). Instead it causes a completely unrelated test to fail. Can you please dig into what's going on here? -Daniel
On Tue, Nov 18, 2014 at 09:45:52AM -0800, Jesse Barnes wrote: > Just like we do in the HDMI code, set the infoframe flag if we detect > that infoframes are enabled. > > v2: check for actual infoframe status as in hdmi code (Daniel) > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Queued for -next, thanks for the patch. Now I need to cry over why QA hasn't found this one :( -Daniel
On Tue, Nov 18, 2014 at 09:45:52AM -0800, Jesse Barnes wrote: > Just like we do in the HDMI code, set the infoframe flag if we detect > that infoframes are enabled. > > v2: check for actual infoframe status as in hdmi code (Daniel) > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_ddi.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 07c5625..24110c9 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2075,6 +2075,14 @@ void intel_ddi_get_config(struct intel_encoder *encoder, > break; > } > > + if (encoder->type == INTEL_OUTPUT_HDMI) { Hm I dind't look too closely apparently at this. You again rely upon sw state here, just encoder->type this time around. Which means you can't upcast the intel_hdmi struct, and you also can't really rely upon the encoder->crtc link (that's all just about to get reconstructed). Imo the code should have stayed in the TRANS_DDI_MODE_SELECT_HDMI case. The later depency upon encoder->crtc is an issue for everything !g4x, but on hsw there's the additional issue that you have to look at the cpu transcoder and I guess that part blows up. g4x infoframe readout is probably broken too because it doesn't check that the port selected is the one actually queried for. Overall I think we need to: - Inline the g4x readout into the hdmi get_config function and check the port. - Inline the ibx/cpt readout code into the relevant get_pipe_config functions (well pch config) since that state is per-pipe. We should probably double-check the port, too. - Same inline for vlv and hsw, with the addition that we need to make sure on hsw to not try to read this for the edp transcoder. Or maybe I'm totally missing why the state gets out of sync on Paulo's machine. -Daniel > + struct intel_hdmi *intel_hdmi = > + enc_to_intel_hdmi(&encoder->base); > + > + if (intel_hdmi->infoframe_enabled(&encoder->base)) > + pipe_config->has_infoframe = true; > + } > + > if (intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_AUDIO)) { > temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); > if (temp & AUDIO_OUTPUT_ENABLE(intel_crtc->pipe)) > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 20 Nov 2014 16:49:42 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Nov 18, 2014 at 09:45:52AM -0800, Jesse Barnes wrote: > > Just like we do in the HDMI code, set the infoframe flag if we detect > > that infoframes are enabled. > > > > v2: check for actual infoframe status as in hdmi code (Daniel) > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 07c5625..24110c9 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2075,6 +2075,14 @@ void intel_ddi_get_config(struct intel_encoder *encoder, > > break; > > } > > > > + if (encoder->type == INTEL_OUTPUT_HDMI) { > > Hm I dind't look too closely apparently at this. You again rely upon sw > state here, just encoder->type this time around. Which means you can't > upcast the intel_hdmi struct, and you also can't really rely upon the > encoder->crtc link (that's all just about to get reconstructed). Imo the > code should have stayed in the TRANS_DDI_MODE_SELECT_HDMI case. Hm, we look at the encoder->type later and check for eDP, so I figured it must be valid at this point. It's easy enough to move though if not... > The later depency upon encoder->crtc is an issue for everything !g4x, but > on hsw there's the additional issue that you have to look at the cpu > transcoder and I guess that part blows up. I'll check out Paulo's trace; haven't looked yet. > g4x infoframe readout is probably broken too because it doesn't check that > the port selected is the one actually queried for. Yeah that looks like a separate bug from the infoframe_enabled additions. > > Overall I think we need to: > - Inline the g4x readout into the hdmi get_config function and check the > port. > - Inline the ibx/cpt readout code into the relevant get_pipe_config > functions (well pch config) since that state is per-pipe. We should > probably double-check the port, too. > - Same inline for vlv and hsw, with the addition that we need to make sure > on hsw to not try to read this for the edp transcoder. I'll check, we may not need to inline since we should be able to get the port info easily enough.
Let's be good citizen and summarize the important points from the irc chat between Jesse&me. On Thu, Nov 20, 2014 at 8:54 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> Hm I dind't look too closely apparently at this. You again rely upon sw >> state here, just encoder->type this time around. Which means you can't >> upcast the intel_hdmi struct, and you also can't really rely upon the >> encoder->crtc link (that's all just about to get reconstructed). Imo the >> code should have stayed in the TRANS_DDI_MODE_SELECT_HDMI case. > > Hm, we look at the encoder->type later and check for eDP, so I figured > it must be valid at this point. It's easy enough to move though if > not... eDP is special since it doesn't change its personality ever. That's why we get away with it. And there's also not really any indication whether a given port would be eDP in the hw since at least on big core the panel power sequencer is global (and not per-pipe like on vlv/bsw) so we just can't tell. Well we could poke at the dp aux stuff. >> The later depency upon encoder->crtc is an issue for everything !g4x, but >> on hsw there's the additional issue that you have to look at the cpu >> transcoder and I guess that part blows up. > > I'll check out Paulo's trace; haven't looked yet. Summary from our irc analysis is in the patch I've just submitted. Hopefully that fixes Paulo's backtrace. >> g4x infoframe readout is probably broken too because it doesn't check that >> the port selected is the one actually queried for. > > Yeah that looks like a separate bug from the infoframe_enabled > additions. > >> >> Overall I think we need to: >> - Inline the g4x readout into the hdmi get_config function and check the >> port. >> - Inline the ibx/cpt readout code into the relevant get_pipe_config >> functions (well pch config) since that state is per-pipe. We should >> probably double-check the port, too. >> - Same inline for vlv and hsw, with the addition that we need to make sure >> on hsw to not try to read this for the edp transcoder. > > I'll check, we may not need to inline since we should be able to get > the port info easily enough. Per our discussion encoder->crtc gets reconstructed properly so we should be able to rely on that relationship. Might be good paranoid practice to double-check the port mapping for ibx/vlv, but imo not really required. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 07c5625..24110c9 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2075,6 +2075,14 @@ void intel_ddi_get_config(struct intel_encoder *encoder, break; } + if (encoder->type == INTEL_OUTPUT_HDMI) { + struct intel_hdmi *intel_hdmi = + enc_to_intel_hdmi(&encoder->base); + + if (intel_hdmi->infoframe_enabled(&encoder->base)) + pipe_config->has_infoframe = true; + } + if (intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_AUDIO)) { temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); if (temp & AUDIO_OUTPUT_ENABLE(intel_crtc->pipe))
Just like we do in the HDMI code, set the infoframe flag if we detect that infoframes are enabled. v2: check for actual infoframe status as in hdmi code (Daniel) Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_ddi.c | 8 ++++++++ 1 file changed, 8 insertions(+)