| Message ID | 1425891578-24778-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 09 Mar 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: > When enabling pipe C, the check for the number of lanes pipe B uses was > ignored in case pipe B wasn't active. This would allow pipe C to be > configured while pipe B is in DPMS off state even if it used more than 2 > lanes. Making pipe B active again while pipe C was also active would > then fail. Seems like a good catch. Broken when, or since forever? Cc: stable? Bugzillas? BR, Jani. > > Tested-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 597c10b..4008bf4 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3150,8 +3150,7 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc) > > static bool pipe_has_enabled_pch(struct intel_crtc *crtc) > { > - return crtc->base.state->enable && crtc->active && > - crtc->config->has_pch_encoder; > + return crtc->base.state->enable && crtc->config->has_pch_encoder; > } > > static void ivb_modeset_global_resources(struct drm_device *dev) > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2015-03-09 at 11:24 +0200, Jani Nikula wrote: > On Mon, 09 Mar 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: > > When enabling pipe C, the check for the number of lanes pipe B uses was > > ignored in case pipe B wasn't active. This would allow pipe C to be > > configured while pipe B is in DPMS off state even if it used more than 2 > > lanes. Making pipe B active again while pipe C was also active would > > then fail. > > Seems like a good catch. Broken when, or since forever? Cc: stable? > Bugzillas? I had to touch this code in the last patch series I submitted, and I raised a concern that this might do the wrong thing. Daniel suggested a tried the test case I described above, which indeed does fail. I haven't done the actual history digging until a minute ago, which turns out quite interesting. The exact opposite of this patch was done in the following patch: commit 1fbc0d789d12fec313c91912fc11733fdfbab863 Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Tue Oct 29 12:04:08 2013 +0100 drm/i915: Fix the PPT fdi lane bifurcate state handling on ivb I'm not sure how much has changed since then, or if the comments on that commit's message are still relevant. Particularly, if the unifying of mode set and dpms on code was ever done, and if it has any effect here. Ander. > > > > Tested-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 597c10b..4008bf4 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -3150,8 +3150,7 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc) > > > > static bool pipe_has_enabled_pch(struct intel_crtc *crtc) > > { > > - return crtc->base.state->enable && crtc->active && > > - crtc->config->has_pch_encoder; > > + return crtc->base.state->enable && crtc->config->has_pch_encoder; > > } > > > > static void ivb_modeset_global_resources(struct drm_device *dev) > > -- > > 2.1.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5915
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -1 282/282 281/282
ILK 308/308 308/308
SNB 307/307 307/307
IVB -2 375/375 373/375
BYT 294/294 294/294
HSW 385/385 385/385
BDW 315/315 315/315
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*PNV igt_gem_userptr_blits_minor-unsync-normal PASS(2) DMESG_WARN(1)PASS(1)
*IVB igt_drv_debugfs_reader PASS(2) DMESG_WARN(2)
*IVB igt_drv_hangman_error-state-sysfs-entry PASS(2) TIMEOUT(2)
Note: You need to pay more attention to line start with '*'
On Mon, Mar 09, 2015 at 11:33:57AM +0200, Ander Conselvan De Oliveira wrote: > On Mon, 2015-03-09 at 11:24 +0200, Jani Nikula wrote: > > On Mon, 09 Mar 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: > > > When enabling pipe C, the check for the number of lanes pipe B uses was > > > ignored in case pipe B wasn't active. This would allow pipe C to be > > > configured while pipe B is in DPMS off state even if it used more than 2 > > > lanes. Making pipe B active again while pipe C was also active would > > > then fail. > > > > Seems like a good catch. Broken when, or since forever? Cc: stable? > > Bugzillas? > > I had to touch this code in the last patch series I submitted, and I > raised a concern that this might do the wrong thing. Daniel suggested a > tried the test case I described above, which indeed does fail. I haven't > done the actual history digging until a minute ago, which turns out > quite interesting. The exact opposite of this patch was done in the > following patch: > > commit 1fbc0d789d12fec313c91912fc11733fdfbab863 > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Tue Oct 29 12:04:08 2013 +0100 > > drm/i915: Fix the PPT fdi lane bifurcate state handling on ivb > > I'm not sure how much has changed since then, or if the comments on that > commit's message are still relevant. Particularly, if the unifying of > mode set and dpms on code was ever done, and if it has any effect here. Indeed your patch would break things again I think for the case. What we essentially want to know is whether we've had to do a modeset change on a given pipe that might require us to update the bifurcate state. We abuse intel->active as a cheap way to tell since for the case we're interested in we have crtc->base.enabled == true and crtc->active == false. That's the case where the pipe will be enabled again, but has been shut down to change the mode. With atomic we need to probably look at crtc_state->mode_changed. As an interim solution we have the same information available in the modeset_pipes bitmask. I think replacing the check for intel_crtc->active with !(modeset_pipes & (1<<intel_crtc->pipe)) should work. The way to reproduce the original bug should be: - Light up pipe B with something requiring more than 2 lanes. - Do an immediate modeset on pipe B to a mode requiring at most 2 lanes. Not that SNA/X always does an interim modeset to everything off, you'd need to code up an igt I think. Or vt-switch between X servers with different modes. Without the intel_crtc->active check we won't set the bifurcate bit. - Try to light up pipe C and go boom on the bifurcate consistency checks. Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 597c10b..4008bf4 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3150,8 +3150,7 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc) static bool pipe_has_enabled_pch(struct intel_crtc *crtc) { - return crtc->base.state->enable && crtc->active && - crtc->config->has_pch_encoder; + return crtc->base.state->enable && crtc->config->has_pch_encoder; } static void ivb_modeset_global_resources(struct drm_device *dev)