| Message ID | 1425579739-17007-1-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote: > Update the hot plug function to handle the SST case. Instead of placing > the SST case within the long/short pulse block, it is now handled after > determining that MST mode is not in use. This way, the topology management > layer can handle any MST-related operations while SST operations are still > correctly handled afterwards. > > This patch also corrects the problem of SST mode only being handled in the > case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes > both short and long pulses are used by the different tests, thus both cases > need to be addressed for SST. > > This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the > previous compliance testing patch sequence. Review feedback on that patch > indicated that updating intel_dp_hot_plug() was not the correct place for > the test handler. > > For the SST case, the main stream is disabled for long HPD pulses as this > generally indicates either a connect/disconnect event or link failure. For > a number of case in compliance testing, the source is required to disable > the main link upon detection of a long HPD. > > V2: > - N/A > V3: > - Place the SST mode link status check into the mst_fail case > - Remove obsolete comment regarding SST mode operation > - Removed an erroneous line of code that snuck in during rebasing > V4: > - Added a disable of the main stream (DP transport) for the long pulse case > for SST to support compliance testing > > Signed-off-by: Todd PRevite <tprevite@gmail.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 080cc23..2460d14 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4618,16 +4618,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > if (intel_dp_check_mst_status(intel_dp) == -EINVAL) > goto mst_fail; > } > - > - if (!intel_dp->is_mst) { > - /* > - * we'll check the link status via the normal hot plug path later - > - * but for short hpds we should check it now > - */ > - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - intel_dp_check_link_status(intel_dp); > - drm_modeset_unlock(&dev->mode_config.connection_mutex); > - } > } > > ret = IRQ_HANDLED; > @@ -4639,6 +4629,21 @@ mst_fail: > DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state); > intel_dp->is_mst = false; > drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst); > + } else { > + /* SST mode - handle short/long pulses here */ > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + /* Clear compliance testing flags/data here to prevent > + * false detection in userspace */ > + intel_dp->compliance_test_data = 0; > + intel_dp->compliance_testing_active = 0; > + /* For a long pulse in SST mode, disable the main link */ > + if (long_hpd) { > + I915_WRITE(DP_TP_CTL(intel_dig_port->port), > + ~DP_TP_CTL_ENABLE); > + } Disabling the main link should be done in userspace. All long pulse requests should be forwarded to userspace as a hotplug event. Userspace can then react to that hotplug appropriately. This way we can again exercise the normal operation of all our dp code. If otoh this is again a case where compliance requirements are so strict that they don't allow any lee-way (I can't come up with a scenario really) then I think this should be in a completely separate commit with the full explanation in the commit message. -Daniel > + intel_dp_check_link_status(intel_dp); > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > + ret = IRQ_HANDLED; > } > put_power: > intel_display_power_put(dev_priv, power_domain); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 03/06/2015 08:34 AM, Daniel Vetter wrote: > On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote: >> Update the hot plug function to handle the SST case. Instead of placing >> the SST case within the long/short pulse block, it is now handled after >> determining that MST mode is not in use. This way, the topology management >> layer can handle any MST-related operations while SST operations are still >> correctly handled afterwards. >> >> This patch also corrects the problem of SST mode only being handled in the >> case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes >> both short and long pulses are used by the different tests, thus both cases >> need to be addressed for SST. >> >> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the >> previous compliance testing patch sequence. Review feedback on that patch >> indicated that updating intel_dp_hot_plug() was not the correct place for >> the test handler. >> >> For the SST case, the main stream is disabled for long HPD pulses as this >> generally indicates either a connect/disconnect event or link failure. For >> a number of case in compliance testing, the source is required to disable >> the main link upon detection of a long HPD. >> >> V2: >> - N/A >> V3: >> - Place the SST mode link status check into the mst_fail case >> - Remove obsolete comment regarding SST mode operation >> - Removed an erroneous line of code that snuck in during rebasing >> V4: >> - Added a disable of the main stream (DP transport) for the long pulse case >> for SST to support compliance testing >> >> Signed-off-by: Todd PRevite <tprevite@gmail.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++---------- >> 1 file changed, 15 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 080cc23..2460d14 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4618,16 +4618,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) >> if (intel_dp_check_mst_status(intel_dp) == -EINVAL) >> goto mst_fail; >> } >> - >> - if (!intel_dp->is_mst) { >> - /* >> - * we'll check the link status via the normal hot plug path later - >> - * but for short hpds we should check it now >> - */ >> - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> - intel_dp_check_link_status(intel_dp); >> - drm_modeset_unlock(&dev->mode_config.connection_mutex); >> - } >> } >> >> ret = IRQ_HANDLED; >> @@ -4639,6 +4629,21 @@ mst_fail: >> DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state); >> intel_dp->is_mst = false; >> drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst); >> + } else { >> + /* SST mode - handle short/long pulses here */ >> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> + /* Clear compliance testing flags/data here to prevent >> + * false detection in userspace */ >> + intel_dp->compliance_test_data = 0; >> + intel_dp->compliance_testing_active = 0; >> + /* For a long pulse in SST mode, disable the main link */ >> + if (long_hpd) { >> + I915_WRITE(DP_TP_CTL(intel_dig_port->port), >> + ~DP_TP_CTL_ENABLE); >> + } > > Disabling the main link should be done in userspace. All long pulse > requests should be forwarded to userspace as a hotplug event. Userspace > can then react to that hotplug appropriately. This way we can again > exercise the normal operation of all our dp code. What's your concern here? Do you want to make sure we get coverage on dp_link_down()? It looks like that might be safe to use here instead of flipping the disable bit directly. Or did you want to go through the whole pipe/port shutdown sequence as well? If so, I think the dpms tests will already cover that, separate from simple compliance. Jesse
On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote: > On 03/06/2015 08:34 AM, Daniel Vetter wrote: > > On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote: > >> Update the hot plug function to handle the SST case. Instead of placing > >> the SST case within the long/short pulse block, it is now handled after > >> determining that MST mode is not in use. This way, the topology management > >> layer can handle any MST-related operations while SST operations are still > >> correctly handled afterwards. > >> > >> This patch also corrects the problem of SST mode only being handled in the > >> case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes > >> both short and long pulses are used by the different tests, thus both cases > >> need to be addressed for SST. > >> > >> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the > >> previous compliance testing patch sequence. Review feedback on that patch > >> indicated that updating intel_dp_hot_plug() was not the correct place for > >> the test handler. > >> > >> For the SST case, the main stream is disabled for long HPD pulses as this > >> generally indicates either a connect/disconnect event or link failure. For > >> a number of case in compliance testing, the source is required to disable > >> the main link upon detection of a long HPD. > >> > >> V2: > >> - N/A > >> V3: > >> - Place the SST mode link status check into the mst_fail case > >> - Remove obsolete comment regarding SST mode operation > >> - Removed an erroneous line of code that snuck in during rebasing > >> V4: > >> - Added a disable of the main stream (DP transport) for the long pulse case > >> for SST to support compliance testing > >> > >> Signed-off-by: Todd PRevite <tprevite@gmail.com> > >> --- > >> drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++---------- > >> 1 file changed, 15 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> index 080cc23..2460d14 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -4618,16 +4618,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > >> if (intel_dp_check_mst_status(intel_dp) == -EINVAL) > >> goto mst_fail; > >> } > >> - > >> - if (!intel_dp->is_mst) { > >> - /* > >> - * we'll check the link status via the normal hot plug path later - > >> - * but for short hpds we should check it now > >> - */ > >> - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > >> - intel_dp_check_link_status(intel_dp); > >> - drm_modeset_unlock(&dev->mode_config.connection_mutex); > >> - } > >> } > >> > >> ret = IRQ_HANDLED; > >> @@ -4639,6 +4629,21 @@ mst_fail: > >> DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state); > >> intel_dp->is_mst = false; > >> drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst); > >> + } else { > >> + /* SST mode - handle short/long pulses here */ > >> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > >> + /* Clear compliance testing flags/data here to prevent > >> + * false detection in userspace */ > >> + intel_dp->compliance_test_data = 0; > >> + intel_dp->compliance_testing_active = 0; > >> + /* For a long pulse in SST mode, disable the main link */ > >> + if (long_hpd) { > >> + I915_WRITE(DP_TP_CTL(intel_dig_port->port), > >> + ~DP_TP_CTL_ENABLE); > >> + } > > > > Disabling the main link should be done in userspace. All long pulse > > requests should be forwarded to userspace as a hotplug event. Userspace > > can then react to that hotplug appropriately. This way we can again > > exercise the normal operation of all our dp code. > > What's your concern here? Do you want to make sure we get coverage on > dp_link_down()? It looks like that might be safe to use here instead of > flipping the disable bit directly. Or did you want to go through the > whole pipe/port shutdown sequence as well? If so, I think the dpms > tests will already cover that, separate from simple compliance. This is likely to upset the state checker, we've already had some fun with killing the hard dp pipe disable that the hdp code occasionally did. Well, still have. The other reason is that dp compliance testing with special-case code is somewhat pointless, except when the compliance test contracts what real-world experience forces us to do. For these exceptions I'd like that we fully understand them and also document them. Disabling the link on a full hot-unplug is something we can (and most DE actually do) do. -Daniel
On 03/09/2015 10:29 AM, Daniel Vetter wrote: > On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote: >> On 03/06/2015 08:34 AM, Daniel Vetter wrote: >>> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote: >>>> + } else { >>>> + /* SST mode - handle short/long pulses here */ >>>> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >>>> + /* Clear compliance testing flags/data here to prevent >>>> + * false detection in userspace */ >>>> + intel_dp->compliance_test_data = 0; >>>> + intel_dp->compliance_testing_active = 0; >>>> + /* For a long pulse in SST mode, disable the main link */ >>>> + if (long_hpd) { >>>> + I915_WRITE(DP_TP_CTL(intel_dig_port->port), >>>> + ~DP_TP_CTL_ENABLE); >>>> + } >>> >>> Disabling the main link should be done in userspace. All long pulse >>> requests should be forwarded to userspace as a hotplug event. Userspace >>> can then react to that hotplug appropriately. This way we can again >>> exercise the normal operation of all our dp code. >> >> What's your concern here? Do you want to make sure we get coverage on >> dp_link_down()? It looks like that might be safe to use here instead of >> flipping the disable bit directly. Or did you want to go through the >> whole pipe/port shutdown sequence as well? If so, I think the dpms >> tests will already cover that, separate from simple compliance. > > This is likely to upset the state checker, we've already had some fun with > killing the hard dp pipe disable that the hdp code occasionally did. Well, > still have. The other reason is that dp compliance testing with > special-case code is somewhat pointless, except when the compliance test > contracts what real-world experience forces us to do. For these exceptions > I'd like that we fully understand them and also document them. Disabling > the link on a full hot-unplug is something we can (and most DE actually > do) do. If we end up hitting the checker while testing, then yeah it would spew. But I thought this was mainly about testing the DP code, making sure we can up/down links, train at different parameters, etc, not about going through full mode sets all the time... But either way, I agree we should be documenting this behavior so we don't get stuck trying to figure it out later. Jesse
On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote: > On 03/09/2015 10:29 AM, Daniel Vetter wrote: > > On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote: > >> On 03/06/2015 08:34 AM, Daniel Vetter wrote: > >>> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote: > >>>> + } else { > >>>> + /* SST mode - handle short/long pulses here */ > >>>> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > >>>> + /* Clear compliance testing flags/data here to prevent > >>>> + * false detection in userspace */ > >>>> + intel_dp->compliance_test_data = 0; > >>>> + intel_dp->compliance_testing_active = 0; > >>>> + /* For a long pulse in SST mode, disable the main link */ > >>>> + if (long_hpd) { > >>>> + I915_WRITE(DP_TP_CTL(intel_dig_port->port), > >>>> + ~DP_TP_CTL_ENABLE); > >>>> + } > >>> > >>> Disabling the main link should be done in userspace. All long pulse > >>> requests should be forwarded to userspace as a hotplug event. Userspace > >>> can then react to that hotplug appropriately. This way we can again > >>> exercise the normal operation of all our dp code. > >> > >> What's your concern here? Do you want to make sure we get coverage on > >> dp_link_down()? It looks like that might be safe to use here instead of > >> flipping the disable bit directly. Or did you want to go through the > >> whole pipe/port shutdown sequence as well? If so, I think the dpms > >> tests will already cover that, separate from simple compliance. > > > > This is likely to upset the state checker, we've already had some fun with > > killing the hard dp pipe disable that the hdp code occasionally did. Well, > > still have. The other reason is that dp compliance testing with > > special-case code is somewhat pointless, except when the compliance test > > contracts what real-world experience forces us to do. For these exceptions > > I'd like that we fully understand them and also document them. Disabling > > the link on a full hot-unplug is something we can (and most DE actually > > do) do. > > If we end up hitting the checker while testing, then yeah it would spew. > > But I thought this was mainly about testing the DP code, making sure we > can up/down links, train at different parameters, etc, not about going > through full mode sets all the time... > > But either way, I agree we should be documenting this behavior so we > don't get stuck trying to figure it out later. I don't think we should be killing the port like this. It'll also kill the pipe on some platforms and then we get all kinds of pipe stuck warnings. So I think we'd definitely want a more graceful shutdown of things. I thought we actually discussed about going to the other direction, ie. potentially allowing the link to brought up without the pipe and enabling/disabling the pipe independently while the link remains up and running?
On 03/09/2015 02:04 PM, Ville Syrjälä wrote: > On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote: >> On 03/09/2015 10:29 AM, Daniel Vetter wrote: >>> On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote: >>>> On 03/06/2015 08:34 AM, Daniel Vetter wrote: >>>>> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote: >>>>>> + } else { >>>>>> + /* SST mode - handle short/long pulses here */ >>>>>> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >>>>>> + /* Clear compliance testing flags/data here to prevent >>>>>> + * false detection in userspace */ >>>>>> + intel_dp->compliance_test_data = 0; >>>>>> + intel_dp->compliance_testing_active = 0; >>>>>> + /* For a long pulse in SST mode, disable the main link */ >>>>>> + if (long_hpd) { >>>>>> + I915_WRITE(DP_TP_CTL(intel_dig_port->port), >>>>>> + ~DP_TP_CTL_ENABLE); >>>>>> + } >>>>> >>>>> Disabling the main link should be done in userspace. All long pulse >>>>> requests should be forwarded to userspace as a hotplug event. Userspace >>>>> can then react to that hotplug appropriately. This way we can again >>>>> exercise the normal operation of all our dp code. >>>> >>>> What's your concern here? Do you want to make sure we get coverage on >>>> dp_link_down()? It looks like that might be safe to use here instead of >>>> flipping the disable bit directly. Or did you want to go through the >>>> whole pipe/port shutdown sequence as well? If so, I think the dpms >>>> tests will already cover that, separate from simple compliance. >>> >>> This is likely to upset the state checker, we've already had some fun with >>> killing the hard dp pipe disable that the hdp code occasionally did. Well, >>> still have. The other reason is that dp compliance testing with >>> special-case code is somewhat pointless, except when the compliance test >>> contracts what real-world experience forces us to do. For these exceptions >>> I'd like that we fully understand them and also document them. Disabling >>> the link on a full hot-unplug is something we can (and most DE actually >>> do) do. >> >> If we end up hitting the checker while testing, then yeah it would spew. >> >> But I thought this was mainly about testing the DP code, making sure we >> can up/down links, train at different parameters, etc, not about going >> through full mode sets all the time... >> >> But either way, I agree we should be documenting this behavior so we >> don't get stuck trying to figure it out later. > > I don't think we should be killing the port like this. It'll also kill > the pipe on some platforms and then we get all kinds of pipe stuck > warnings. So I think we'd definitely want a more graceful shutdown of > things. Does that affect current platforms? Or just CTG and ILK? I can guess BYT & BSW might be affected, but I haven't tested. As long as we just up/down the port w/o anything else it might be able to work... > I thought we actually discussed about going to the other direction, ie. > potentially allowing the link to brought up without the pipe and > enabling/disabling the pipe independently while the link remains up and > running? I guess I was thinking the reverse: that bringing up the port w/o a pipe driving it would be more likely to cause problems, but I guess we'll need testing. Depending on what we find, we could change the logic to accommodate the platforms we want to test (HSW+ and BYT+ I think, though we could limit it to even newer ones if those are too tough to handle). Jesse
On Wed, Mar 11, 2015 at 11:37:23AM -0700, Jesse Barnes wrote: > On 03/09/2015 02:04 PM, Ville Syrjälä wrote: > > On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote: > >> On 03/09/2015 10:29 AM, Daniel Vetter wrote: > >>> On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote: > >>>> On 03/06/2015 08:34 AM, Daniel Vetter wrote: > >>>>> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote: > >>>>>> + } else { > >>>>>> + /* SST mode - handle short/long pulses here */ > >>>>>> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > >>>>>> + /* Clear compliance testing flags/data here to prevent > >>>>>> + * false detection in userspace */ > >>>>>> + intel_dp->compliance_test_data = 0; > >>>>>> + intel_dp->compliance_testing_active = 0; > >>>>>> + /* For a long pulse in SST mode, disable the main link */ > >>>>>> + if (long_hpd) { > >>>>>> + I915_WRITE(DP_TP_CTL(intel_dig_port->port), > >>>>>> + ~DP_TP_CTL_ENABLE); > >>>>>> + } > >>>>> > >>>>> Disabling the main link should be done in userspace. All long pulse > >>>>> requests should be forwarded to userspace as a hotplug event. Userspace > >>>>> can then react to that hotplug appropriately. This way we can again > >>>>> exercise the normal operation of all our dp code. > >>>> > >>>> What's your concern here? Do you want to make sure we get coverage on > >>>> dp_link_down()? It looks like that might be safe to use here instead of > >>>> flipping the disable bit directly. Or did you want to go through the > >>>> whole pipe/port shutdown sequence as well? If so, I think the dpms > >>>> tests will already cover that, separate from simple compliance. > >>> > >>> This is likely to upset the state checker, we've already had some fun with > >>> killing the hard dp pipe disable that the hdp code occasionally did. Well, > >>> still have. The other reason is that dp compliance testing with > >>> special-case code is somewhat pointless, except when the compliance test > >>> contracts what real-world experience forces us to do. For these exceptions > >>> I'd like that we fully understand them and also document them. Disabling > >>> the link on a full hot-unplug is something we can (and most DE actually > >>> do) do. > >> > >> If we end up hitting the checker while testing, then yeah it would spew. > >> > >> But I thought this was mainly about testing the DP code, making sure we > >> can up/down links, train at different parameters, etc, not about going > >> through full mode sets all the time... > >> > >> But either way, I agree we should be documenting this behavior so we > >> don't get stuck trying to figure it out later. > > > > I don't think we should be killing the port like this. It'll also kill > > the pipe on some platforms and then we get all kinds of pipe stuck > > warnings. So I think we'd definitely want a more graceful shutdown of > > things. > > Does that affect current platforms? Or just CTG and ILK? I can guess > BYT & BSW might be affected, but I haven't tested. As long as we just > up/down the port w/o anything else it might be able to work... I think you have that reversed. Old platforms were generally fine with enabling/disabling ports while the pipe kept running, but I think that changed at some point (with ilk I suppose), and killing the port is then a bad idea. That's at least the case with DP. I seem to recall we had at least stuck pipes (and maybe even some lockups or something?) when we had the dp_link_down() call in the hpd handler. > > > I thought we actually discussed about going to the other direction, ie. > > potentially allowing the link to brought up without the pipe and > > enabling/disabling the pipe independently while the link remains up and > > running? > > I guess I was thinking the reverse: that bringing up the port w/o a pipe > driving it would be more likely to cause problems, but I guess we'll > need testing. Bringing up the port on its own is perfectly fine. We do that for link training already, and if you consider MST you'll notice it's the only way really.
On Wed, Mar 11, 2015 at 09:10:29PM +0200, Ville Syrjälä wrote: > On Wed, Mar 11, 2015 at 11:37:23AM -0700, Jesse Barnes wrote: > > On 03/09/2015 02:04 PM, Ville Syrjälä wrote: > > > On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote: > > >> On 03/09/2015 10:29 AM, Daniel Vetter wrote: > > >>> On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote: > > >>>> On 03/06/2015 08:34 AM, Daniel Vetter wrote: > > >>>>> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote: > > >>>>>> + } else { > > >>>>>> + /* SST mode - handle short/long pulses here */ > > >>>>>> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > >>>>>> + /* Clear compliance testing flags/data here to prevent > > >>>>>> + * false detection in userspace */ > > >>>>>> + intel_dp->compliance_test_data = 0; > > >>>>>> + intel_dp->compliance_testing_active = 0; > > >>>>>> + /* For a long pulse in SST mode, disable the main link */ > > >>>>>> + if (long_hpd) { > > >>>>>> + I915_WRITE(DP_TP_CTL(intel_dig_port->port), > > >>>>>> + ~DP_TP_CTL_ENABLE); > > >>>>>> + } > > >>>>> > > >>>>> Disabling the main link should be done in userspace. All long pulse > > >>>>> requests should be forwarded to userspace as a hotplug event. Userspace > > >>>>> can then react to that hotplug appropriately. This way we can again > > >>>>> exercise the normal operation of all our dp code. > > >>>> > > >>>> What's your concern here? Do you want to make sure we get coverage on > > >>>> dp_link_down()? It looks like that might be safe to use here instead of > > >>>> flipping the disable bit directly. Or did you want to go through the > > >>>> whole pipe/port shutdown sequence as well? If so, I think the dpms > > >>>> tests will already cover that, separate from simple compliance. > > >>> > > >>> This is likely to upset the state checker, we've already had some fun with > > >>> killing the hard dp pipe disable that the hdp code occasionally did. Well, > > >>> still have. The other reason is that dp compliance testing with > > >>> special-case code is somewhat pointless, except when the compliance test > > >>> contracts what real-world experience forces us to do. For these exceptions > > >>> I'd like that we fully understand them and also document them. Disabling > > >>> the link on a full hot-unplug is something we can (and most DE actually > > >>> do) do. > > >> > > >> If we end up hitting the checker while testing, then yeah it would spew. > > >> > > >> But I thought this was mainly about testing the DP code, making sure we > > >> can up/down links, train at different parameters, etc, not about going > > >> through full mode sets all the time... > > >> > > >> But either way, I agree we should be documenting this behavior so we > > >> don't get stuck trying to figure it out later. > > > > > > I don't think we should be killing the port like this. It'll also kill > > > the pipe on some platforms and then we get all kinds of pipe stuck > > > warnings. So I think we'd definitely want a more graceful shutdown of > > > things. > > > > Does that affect current platforms? Or just CTG and ILK? I can guess > > BYT & BSW might be affected, but I haven't tested. As long as we just > > up/down the port w/o anything else it might be able to work... > > I think you have that reversed. Old platforms were generally fine with > enabling/disabling ports while the pipe kept running, but I think that > changed at some point (with ilk I suppose), and killing the port is then > a bad idea. > > That's at least the case with DP. I seem to recall we had at least stuck > pipes (and maybe even some lockups or something?) when we had the > dp_link_down() call in the hpd handler. cpu edp on ilk+ and hsw+ dp are the ones that definitely get cranky if you just kill the port. Not sure whether we've even seen hard hangs in dp enabling on hsw, it's been a very long time ago. Well we've seen hard hangs on hsw because of modeset bugs, but I don't remember whether it was this on here too. > > > I thought we actually discussed about going to the other direction, ie. > > > potentially allowing the link to brought up without the pipe and > > > enabling/disabling the pipe independently while the link remains up and > > > running? > > > > I guess I was thinking the reverse: that bringing up the port w/o a pipe > > driving it would be more likely to cause problems, but I guess we'll > > need testing. > > Bringing up the port on its own is perfectly fine. We do that for link > training already, and if you consider MST you'll notice it's the only > way really. Yeah on hsw+ and cpu edp we bring up the prot first iirc, then fire up the pipe later on. Dunno what we do for external dp on ilk-ivb or what exactly we should be doing - we iirc still have the occasionally dp port stuck bug floating about. In any case there's a very well defined sequence to go about things, and anything else tends to anger the machines a lot. If we can't just use the setCrtc ioctl from userspace we should at least reuse the link shutdown machinery we have properly. Of course I'd prefer if we don't have to since that would be less extra code to keep working. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 080cc23..2460d14 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4618,16 +4618,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) if (intel_dp_check_mst_status(intel_dp) == -EINVAL) goto mst_fail; } - - if (!intel_dp->is_mst) { - /* - * we'll check the link status via the normal hot plug path later - - * but for short hpds we should check it now - */ - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); - drm_modeset_unlock(&dev->mode_config.connection_mutex); - } } ret = IRQ_HANDLED; @@ -4639,6 +4629,21 @@ mst_fail: DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state); intel_dp->is_mst = false; drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst); + } else { + /* SST mode - handle short/long pulses here */ + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); + /* Clear compliance testing flags/data here to prevent + * false detection in userspace */ + intel_dp->compliance_test_data = 0; + intel_dp->compliance_testing_active = 0; + /* For a long pulse in SST mode, disable the main link */ + if (long_hpd) { + I915_WRITE(DP_TP_CTL(intel_dig_port->port), + ~DP_TP_CTL_ENABLE); + } + intel_dp_check_link_status(intel_dp); + drm_modeset_unlock(&dev->mode_config.connection_mutex); + ret = IRQ_HANDLED; } put_power: intel_display_power_put(dev_priv, power_domain);
Update the hot plug function to handle the SST case. Instead of placing the SST case within the long/short pulse block, it is now handled after determining that MST mode is not in use. This way, the topology management layer can handle any MST-related operations while SST operations are still correctly handled afterwards. This patch also corrects the problem of SST mode only being handled in the case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes both short and long pulses are used by the different tests, thus both cases need to be addressed for SST. This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the previous compliance testing patch sequence. Review feedback on that patch indicated that updating intel_dp_hot_plug() was not the correct place for the test handler. For the SST case, the main stream is disabled for long HPD pulses as this generally indicates either a connect/disconnect event or link failure. For a number of case in compliance testing, the source is required to disable the main link upon detection of a long HPD. V2: - N/A V3: - Place the SST mode link status check into the mst_fail case - Remove obsolete comment regarding SST mode operation - Removed an erroneous line of code that snuck in during rebasing V4: - Added a disable of the main stream (DP transport) for the long pulse case for SST to support compliance testing Signed-off-by: Todd PRevite <tprevite@gmail.com> --- drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-)