| Message ID | 1428682372-21586-8-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>: > 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 > V5: > - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 > V6: > - Reformatted a comment > > Signed-off-by: Todd Previte <tprevite@gmail.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++----------- > 1 file changed, 8 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 77b6b15..ba2da44 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4572,29 +4572,26 @@ 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); > - } > } Shouldn't the code be moved to exactly this spot instead of after the put_power label? Why would we want to call check_link_status in case we goto mst_fail? In case there is a valid reason, maybe it would be better to do a big reorganization of this function because it's going to start looking very weird - or at least rename the labels. Also, for the long_hpd case, I see that check_link_status() will redo some of the stuff we already did on this function, such as get_dpcd(). And if you follow my advice on patch 2, you will end up having even more repeated code. I think you could try to do a careful analysis here to make sure we're not calling stuff twice here, especially since some of those operations are potentially slow. > > ret = IRQ_HANDLED; > > goto put_power; > mst_fail: > - /* if we were in MST mode, and device is not there get out of MST mode */ > if (intel_dp->is_mst) { > + /* if we were in MST mode, and device is not there get out of MST mode */ I don't see the need for changes such as the one above - I saw similar cases in other patches you submitted. I often use git blame on comments in order to be able to see the whole context of the change, and a simple change like this makes it harder to blame. Also, you're not even fixing the 80 column problem here. And I do prefer the comment on top of the if statement. > 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); > } > put_power: > + /* SST mode - handle short/long pulses here */ > + if (!intel_dp->is_mst) { > + 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; > + } > intel_display_power_put(dev_priv, power_domain); > > return ret; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 4/14/15 4:29 AM, Paulo Zanoni wrote: > 2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>: >> 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 >> V5: >> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 >> V6: >> - Reformatted a comment >> >> Signed-off-by: Todd Previte <tprevite@gmail.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++----------- >> 1 file changed, 8 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 77b6b15..ba2da44 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4572,29 +4572,26 @@ 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); >> - } >> } > Shouldn't the code be moved to exactly this spot instead of after the > put_power label? Why would we want to call check_link_status in case > we goto mst_fail? In case there is a valid reason, maybe it would be > better to do a big reorganization of this function because it's going > to start looking very weird - or at least rename the labels. No because then you don't get long pulses, only short ones. The put_power case is where this belongs, unless you want to duplicate code in both the long_pulse and the else clause. There is a separate mst_check_link_status call so this one is specific to SST mode. There is also a check to make sure it doesn't get called when MST is active and MST has hit a failure mode, so that is a non-issue. > Also, for the long_hpd case, I see that check_link_status() will redo > some of the stuff we already did on this function, such as get_dpcd(). > And if you follow my advice on patch 2, you will end up having even > more repeated code. I think you could try to do a careful analysis > here to make sure we're not calling stuff twice here, especially since > some of those operations are potentially slow. I see a couple places where the code is duplicated, specifically the connection check (which I encapsulated in a function and I'll likely roll forward into this one since it makes things more clear) and the DPCD read in the long pulse case. I removed the code in check_link_status for both of these things and it still passes compliance. Good catch Paulo. This has been fixed and tested and will be in the updated patch posted shortly. >> ret = IRQ_HANDLED; >> >> goto put_power; >> mst_fail: >> - /* if we were in MST mode, and device is not there get out of MST mode */ >> if (intel_dp->is_mst) { >> + /* if we were in MST mode, and device is not there get out of MST mode */ > I don't see the need for changes such as the one above - I saw similar > cases in other patches you submitted. I often use git blame on > comments in order to be able to see the whole context of the change, > and a simple change like this makes it harder to blame. Also, you're > not even fixing the 80 column problem here. And I do prefer the > comment on top of the if statement. This is just an artifact of moving things around, as it likely was in the other patches. The only reason I will move comments is to clarify what they pertain to if code is moving around it. It's back where it belongs now so it doesn't even show up in the patch. Fixed for the next version. > >> 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); >> } >> put_power: >> + /* SST mode - handle short/long pulses here */ >> + if (!intel_dp->is_mst) { >> + 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; >> + } >> intel_display_power_put(dev_priv, power_domain); >> >> return ret; >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
2015-04-14 14:36 GMT-03:00 Todd Previte <tprevite@gmail.com>: > > > On 4/14/15 4:29 AM, Paulo Zanoni wrote: >> >> 2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>: >>> >>> 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 >>> V5: >>> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 >>> V6: >>> - Reformatted a comment >>> >>> Signed-off-by: Todd Previte <tprevite@gmail.com> >>> --- >>> drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++----------- >>> 1 file changed, 8 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>> b/drivers/gpu/drm/i915/intel_dp.c >>> index 77b6b15..ba2da44 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -4572,29 +4572,26 @@ 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); >>> - } >>> } >> >> Shouldn't the code be moved to exactly this spot instead of after the >> put_power label? Why would we want to call check_link_status in case >> we goto mst_fail? In case there is a valid reason, maybe it would be >> better to do a big reorganization of this function because it's going >> to start looking very weird - or at least rename the labels. > > No because then you don't get long pulses, only short ones. No, what I mean is: if (long_hpd) { ... code ... } else { ... code .... } if (!intel_dp->is_mst) { drm_modeset_lock(...) ... code ... } mst_fail: ... code ... The other problem I point is: imagine we're SST and we get a long_hpd. Then we run ibx_digital_port_connected(), and since the monitor is disconnected we "goto mst_fail". We'll end up running intel_dp_check_link_status() before returning, but we really shouldn't run it since we know the monitor is disconnected. > The put_power > case is where this belongs, unless you want to duplicate code in both the > long_pulse and the else clause. There is a separate mst_check_link_status > call so this one is specific to SST mode. There is also a check to make sure > it doesn't get called when MST is active and MST has hit a failure mode, so > that is a non-issue. >> >> Also, for the long_hpd case, I see that check_link_status() will redo >> some of the stuff we already did on this function, such as get_dpcd(). >> And if you follow my advice on patch 2, you will end up having even >> more repeated code. I think you could try to do a careful analysis >> here to make sure we're not calling stuff twice here, especially since >> some of those operations are potentially slow. > > I see a couple places where the code is duplicated, specifically the > connection check (which I encapsulated in a function and I'll likely roll > forward into this one since it makes things more clear) and the DPCD read in > the long pulse case. I removed the code in check_link_status for both of > these things and it still passes compliance. Good catch Paulo. This has been > fixed and tested and will be in the updated patch posted shortly. >>> >>> ret = IRQ_HANDLED; >>> >>> goto put_power; >>> mst_fail: >>> - /* if we were in MST mode, and device is not there get out of MST >>> mode */ >>> if (intel_dp->is_mst) { >>> + /* if we were in MST mode, and device is not there get >>> out of MST mode */ >> >> I don't see the need for changes such as the one above - I saw similar >> cases in other patches you submitted. I often use git blame on >> comments in order to be able to see the whole context of the change, >> and a simple change like this makes it harder to blame. Also, you're >> not even fixing the 80 column problem here. And I do prefer the >> comment on top of the if statement. > > This is just an artifact of moving things around, as it likely was in the > other patches. The only reason I will move comments is to clarify what they > pertain to if code is moving around it. It's back where it belongs now so it > doesn't even show up in the patch. Fixed for the next version. > >> >>> 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); >>> } >>> put_power: >>> + /* SST mode - handle short/long pulses here */ >>> + if (!intel_dp->is_mst) { >>> + 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; >>> + } >>> intel_display_power_put(dev_priv, power_domain); >>> >>> return ret; >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >
On 4/14/15 12:00 PM, Paulo Zanoni wrote: > 2015-04-14 14:36 GMT-03:00 Todd Previte <tprevite@gmail.com>: >> >> On 4/14/15 4:29 AM, Paulo Zanoni wrote: >>> 2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite@gmail.com>: >>>> 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 >>>> V5: >>>> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 >>>> V6: >>>> - Reformatted a comment >>>> >>>> Signed-off-by: Todd Previte <tprevite@gmail.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++----------- >>>> 1 file changed, 8 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>>> b/drivers/gpu/drm/i915/intel_dp.c >>>> index 77b6b15..ba2da44 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>> @@ -4572,29 +4572,26 @@ 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); >>>> - } >>>> } >>> Shouldn't the code be moved to exactly this spot instead of after the >>> put_power label? Why would we want to call check_link_status in case >>> we goto mst_fail? In case there is a valid reason, maybe it would be >>> better to do a big reorganization of this function because it's going >>> to start looking very weird - or at least rename the labels. >> No because then you don't get long pulses, only short ones. > No, what I mean is: > > if (long_hpd) { > ... code ... > } else { > ... code .... > } > > if (!intel_dp->is_mst) { > drm_modeset_lock(...) > ... code ... > } > > mst_fail: > ... code ... > > The other problem I point is: imagine we're SST and we get a long_hpd. > Then we run ibx_digital_port_connected(), and since the monitor is > disconnected we "goto mst_fail". We'll end up running > intel_dp_check_link_status() before returning, but we really shouldn't > run it since we know the monitor is disconnected. > I see what you're saying, however under normal operation for SST (when connected and everything is working) the code will hit this line: if (!intel_dp_probe_mst(intel_dp)) And proceed to the mst_fail block, thus skipping that block of code entirely and missing the SST handler. The result is a missed long pulse for the SST case. Your second point has validity though. This can be addressed with a "connected" flag just before the if (long_pulse) statement: connected = intel_dp_digital_port_connected(intel_dp); if (!connected) goto mst_fail; The pulse handler for the most part can then be skipped, since the device is gone. In mst_fail, the MST topology manager is updated though, so that still has to happen. With the SST pulse handler in put_power, it can simply fall through now and hit the updated if-statement there which is: if (!intel_dp->is_mst && connected) { ... code ... } And all should be well for a disconnected device as well as normal operational modes of SST and MST. Oh the intel_dp_digital_port_connected() function is just the encapsulation of this code from the long_pulse segment: if (HAS_PCH_SPLIT(dev)) { if (!ibx_digital_port_connected(dev_priv, intel_dig_port)) goto mst_fail; } else { if (g4x_digital_port_connected(dev, intel_dig_port) != 1) goto mst_fail; } And it's been added to the updated patch. >> The put_power >> case is where this belongs, unless you want to duplicate code in both the >> long_pulse and the else clause. There is a separate mst_check_link_status >> call so this one is specific to SST mode. There is also a check to make sure >> it doesn't get called when MST is active and MST has hit a failure mode, so >> that is a non-issue. >>> Also, for the long_hpd case, I see that check_link_status() will redo >>> some of the stuff we already did on this function, such as get_dpcd(). >>> And if you follow my advice on patch 2, you will end up having even >>> more repeated code. I think you could try to do a careful analysis >>> here to make sure we're not calling stuff twice here, especially since >>> some of those operations are potentially slow. >> I see a couple places where the code is duplicated, specifically the >> connection check (which I encapsulated in a function and I'll likely roll >> forward into this one since it makes things more clear) and the DPCD read in >> the long pulse case. I removed the code in check_link_status for both of >> these things and it still passes compliance. Good catch Paulo. This has been >> fixed and tested and will be in the updated patch posted shortly. >>>> ret = IRQ_HANDLED; >>>> >>>> goto put_power; >>>> mst_fail: >>>> - /* if we were in MST mode, and device is not there get out of MST >>>> mode */ >>>> if (intel_dp->is_mst) { >>>> + /* if we were in MST mode, and device is not there get >>>> out of MST mode */ >>> I don't see the need for changes such as the one above - I saw similar >>> cases in other patches you submitted. I often use git blame on >>> comments in order to be able to see the whole context of the change, >>> and a simple change like this makes it harder to blame. Also, you're >>> not even fixing the 80 column problem here. And I do prefer the >>> comment on top of the if statement. >> This is just an artifact of moving things around, as it likely was in the >> other patches. The only reason I will move comments is to clarify what they >> pertain to if code is moving around it. It's back where it belongs now so it >> doesn't even show up in the patch. Fixed for the next version. >> >>>> 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); >>>> } >>>> put_power: >>>> + /* SST mode - handle short/long pulses here */ >>>> + if (!intel_dp->is_mst) { >>>> + 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; >>>> + } >>>> intel_display_power_put(dev_priv, power_domain); >>>> >>>> return ret; >>>> -- >>>> 1.9.1 >>>> >>>> _______________________________________________ >>>> 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 77b6b15..ba2da44 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4572,29 +4572,26 @@ 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; goto put_power; mst_fail: - /* if we were in MST mode, and device is not there get out of MST mode */ if (intel_dp->is_mst) { + /* if we were in MST mode, and device is not there get out of MST mode */ 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); } put_power: + /* SST mode - handle short/long pulses here */ + if (!intel_dp->is_mst) { + 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; + } intel_display_power_put(dev_priv, power_domain); return ret;
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 V5: - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 V6: - Reformatted a comment Signed-off-by: Todd Previte <tprevite@gmail.com> --- drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)