| Message ID | 1418255597-4716-14-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>: > Moves the non-MST case out of the if-statement and places it at the beginning > of the function to handle HPD events for SST mode. The reasoning behind this > is to accommodate link status checks for compliance testing. Some test devices > use long pulses to perform test requests so link status must be checked > regardless of the pulse width for the SST operational mode. Can you please elaborate a little more on what do you see on these devices? The test spec is very clear about short vs long HPD pulses, so it's hard to believe a test device would get this wrong. We have some registers on the PCH that allow us to redefine short vs long durations. Have you tried to play with them? More below: > > 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. > > Signed-off-by: Todd Previte <tprevite@gmail.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 4a55ca6..73014d8 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > power_domain = intel_display_port_power_domain(intel_encoder); > intel_display_power_get(dev_priv, power_domain); > > + if (!intel_dp->is_mst) { > + /* > + * Pulse width doesn't matter for SST mode > + * Handle the HPD event now > + */ > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + intel_dp_check_link_status(intel_dp); The very first thing intel_dp_check_link_status() does is to return in case "connector->base.status != connected". If we're getting a long HPD, it doesn't seem make sense to check this field because the status might be changing due to the long HPD. > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > + ret = false; > + goto put_power; > + } > + > if (long_hpd) { > > if (HAS_PCH_SPLIT(dev)) { > @@ -4637,16 +4649,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 = false; > goto put_power; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Dec 17, 2014 at 03:57:21PM -0200, Paulo Zanoni wrote: > 2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>: > > Moves the non-MST case out of the if-statement and places it at the beginning > > of the function to handle HPD events for SST mode. The reasoning behind this > > is to accommodate link status checks for compliance testing. Some test devices > > use long pulses to perform test requests so link status must be checked > > regardless of the pulse width for the SST operational mode. > > Can you please elaborate a little more on what do you see on these > devices? The test spec is very clear about short vs long HPD pulses, > so it's hard to believe a test device would get this wrong. We have > some registers on the PCH that allow us to redefine short vs long > durations. Have you tried to play with them? > > More below: > > > > > 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. > > > > Signed-off-by: Todd Previte <tprevite@gmail.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++---------- > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 4a55ca6..73014d8 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > > power_domain = intel_display_port_power_domain(intel_encoder); > > intel_display_power_get(dev_priv, power_domain); > > > > + if (!intel_dp->is_mst) { > > + /* > > + * Pulse width doesn't matter for SST mode > > + * Handle the HPD event now > > + */ > > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > + intel_dp_check_link_status(intel_dp); > > The very first thing intel_dp_check_link_status() does is to return in > case "connector->base.status != connected". If we're getting a long > HPD, it doesn't seem make sense to check this field because the status > might be changing due to the long HPD. I don't think we can unconditionally run SST hpd logic before we've correctly handled mst mode. It likely screws up the accounting. > > > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > > + ret = false; > > + goto put_power; > > + } > > + > > if (long_hpd) { > > > > if (HAS_PCH_SPLIT(dev)) { > > @@ -4637,16 +4649,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 > > - */ Just aside: The above comment is outdated and can be remove. This is now the only place where we handle link retraining. The function could be made static and dropped from headers, too. -Daniel > > - 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 = false; > > goto put_power; > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 12/17/14 10:57 AM, Paulo Zanoni wrote: > 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>: >> Moves the non-MST case out of the if-statement and places it at the beginning >> of the function to handle HPD events for SST mode. The reasoning behind this >> is to accommodate link status checks for compliance testing. Some test devices >> use long pulses to perform test requests so link status must be checked >> regardless of the pulse width for the SST operational mode. > Can you please elaborate a little more on what do you see on these > devices? The test spec is very clear about short vs long HPD pulses, > so it's hard to believe a test device would get this wrong. We have > some registers on the PCH that allow us to redefine short vs long > durations. Have you tried to play with them? > > More below: The issue is not in differentiating between the two pulse widths. The problem is that compliance testing mixes the two together, i.e. some tests are IRQ events where the source has to "service" the sink while other tests are examining the hot plug detection and response functionality. So when it comes to compliance testing, they both have to be handled and checked to see if there's a test request coming in or if it's a real HPD event of some kind. The current implementation only checks SST mode for the short pulse case. In light of Daniel's comment, though, that makes this code even more broken. The best place to handle this (as I mention in my response to Daniel) is to place the SST case after the mst_fail tag. That way, once it is determined that MST mode is not in use, the SST handler can be invoked and events responded to appropriately. >> 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. >> >> Signed-off-by: Todd Previte<tprevite@gmail.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++---------- >> 1 file changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 4a55ca6..73014d8 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) >> power_domain = intel_display_port_power_domain(intel_encoder); >> intel_display_power_get(dev_priv, power_domain); >> >> + if (!intel_dp->is_mst) { >> + /* >> + * Pulse width doesn't matter for SST mode >> + * Handle the HPD event now >> + */ >> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> + intel_dp_check_link_status(intel_dp); > The very first thing intel_dp_check_link_status() does is to return in > case "connector->base.status != connected". If we're getting a long > HPD, it doesn't seem make sense to check this field because the status > might be changing due to the long HPD. Long pulses are either connect OR disconnect events. In the case of the test device, what happens is that it's already connected, so HPD is asserted. It pulses the HPD line low for > 2ms and then reasserts it sometime later once the source device has disabled the main link. So it becomes a 2-fold event for us - the initial connected->disconnected transition and the following disconnected->connected transition. Both of those invoke our IRQ handler and cause neat and exciting things to happen. For compliance testing, the disconnect event needs to be ignored (by the test code, anyways) and the connect event needs to be checked for TEST_REQUEST=1. Thus, it absolutely makes sense to check this here. >> + drm_modeset_unlock(&dev->mode_config.connection_mutex); >> + ret = false; >> + goto put_power; >> + } >> + >> if (long_hpd) { >> >> if (HAS_PCH_SPLIT(dev)) { >> @@ -4637,16 +4649,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 = false; >> goto put_power; >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On 12/17/14 1:30 PM, Daniel Vetter wrote: > On Wed, Dec 17, 2014 at 03:57:21PM -0200, Paulo Zanoni wrote: >> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>: >>> Moves the non-MST case out of the if-statement and places it at the beginning >>> of the function to handle HPD events for SST mode. The reasoning behind this >>> is to accommodate link status checks for compliance testing. Some test devices >>> use long pulses to perform test requests so link status must be checked >>> regardless of the pulse width for the SST operational mode. >> Can you please elaborate a little more on what do you see on these >> devices? The test spec is very clear about short vs long HPD pulses, >> so it's hard to believe a test device would get this wrong. We have >> some registers on the PCH that allow us to redefine short vs long >> durations. Have you tried to play with them? >> >> More below: >> >>> 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. >>> >>> Signed-off-by: Todd Previte<tprevite@gmail.com> >>> --- >>> drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++---------- >>> 1 file changed, 12 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>> index 4a55ca6..73014d8 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) >>> power_domain = intel_display_port_power_domain(intel_encoder); >>> intel_display_power_get(dev_priv, power_domain); >>> >>> + if (!intel_dp->is_mst) { >>> + /* >>> + * Pulse width doesn't matter for SST mode >>> + * Handle the HPD event now >>> + */ >>> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >>> + intel_dp_check_link_status(intel_dp); >> The very first thing intel_dp_check_link_status() does is to return in >> case "connector->base.status != connected". If we're getting a long >> HPD, it doesn't seem make sense to check this field because the status >> might be changing due to the long HPD. > I don't think we can unconditionally run SST hpd logic before we've > correctly handled mst mode. It likely screws up the accounting. Upon further review, it looks like the best solution here is to place the SST code in the mst_fail case as the 'else' clause. That way SST mode stuff gets handled regardless and if a need arises in the future where differentiating between short and long becomes necessary, there's a place to handle it. This will be in V3. >>> + drm_modeset_unlock(&dev->mode_config.connection_mutex); >>> + ret = false; >>> + goto put_power; >>> + } >>> + >>> if (long_hpd) { >>> >>> if (HAS_PCH_SPLIT(dev)) { >>> @@ -4637,16 +4649,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 >>> - */ > Just aside: The above comment is outdated and can be remove. This is now > the only place where we handle link retraining. The function could be made > static and dropped from headers, too. > -Daniel Can't be static and pulled from the header. It's called in intel_ddi.c as well. But the comment and the code is gone, in light of the above changes. >>> - 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 = false; >>> goto put_power; >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> -- >> Paulo Zanoni >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 4a55ca6..73014d8 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) power_domain = intel_display_port_power_domain(intel_encoder); intel_display_power_get(dev_priv, power_domain); + if (!intel_dp->is_mst) { + /* + * Pulse width doesn't matter for SST mode + * Handle the HPD event 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 = false; + goto put_power; + } + if (long_hpd) { if (HAS_PCH_SPLIT(dev)) { @@ -4637,16 +4649,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 = false; goto put_power;
Moves the non-MST case out of the if-statement and places it at the beginning of the function to handle HPD events for SST mode. The reasoning behind this is to accommodate link status checks for compliance testing. Some test devices use long pulses to perform test requests so link status must be checked regardless of the pulse width for the SST operational mode. 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. Signed-off-by: Todd Previte <tprevite@gmail.com> --- drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)