| Message ID | 1453199833-10401-2-git-send-email-shubhangi.shrivastava@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote: > Current DP detection has DPCD operations split across > intel_dp_hpd_pulse and intel_dp_detect which contains > duplicates as well. Also intel_dp_detect is called > during modes enumeration as well which will result > in multiple dpcd operations. So this patch tries > to solve both these by bringing all DPCD operations > in one single function and make intel_dp_detect > use existing values instead of repeating same steps. > > v2: Pulled in a hunk from last patch of the series to > this patch. (Ander) > v3: Added MST hotplug handling. (Ander) Please pick up previous review tags when resending patches. In this case, this was already Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++--------------- > - > 1 file changed, 44 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 8969ff9..82ee18d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4638,8 +4638,19 @@ intel_dp_long_pulse(struct intel_connector > *intel_connector) > intel_dp_probe_oui(intel_dp); > > ret = intel_dp_probe_mst(intel_dp); > - if (ret) > + if (ret) { > + goto out; > + } else if (connector->status == connector_status_connected) { > + /* > + * If display was connected already and is still connected > + * check links status, there has been known issues of > + * link loss triggerring long pulse!!!! > + */ > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + intel_dp_check_link_status(intel_dp); > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > goto out; > + } > > /* > * Clearing NACK and defer counts to get their exact values > @@ -4668,8 +4679,21 @@ intel_dp_long_pulse(struct intel_connector > *intel_connector) > } > > out: > - if (status != connector_status_connected) > + if (status != connector_status_connected) { > intel_dp_unset_edid(intel_dp); > + /* > + * If we were in MST mode, and device is not there, > + * get out of MST mode > + */ > + if (intel_dp->is_mst) { > + 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); > + } > + } > + > intel_display_power_put(to_i915(dev), power_domain); > return; > } > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > return connector_status_disconnected; > } > > - intel_dp_long_pulse(intel_dp->attached_connector); > + if (force) > + intel_dp_long_pulse(intel_dp->attached_connector); > > if (intel_connector->detect_edid) > return connector_status_connected; > @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > /* indicate that we need to restart link training */ > intel_dp->train_set_valid = false; > > - if (!intel_digital_port_connected(dev_priv, intel_dig_port)) > - goto mst_fail; > - > - if (!intel_dp_get_dpcd(intel_dp)) { > - goto mst_fail; > - } > - > - intel_dp_probe_oui(intel_dp); > + intel_dp_long_pulse(intel_dp->attached_connector); > + if (intel_dp->is_mst) > + ret = IRQ_HANDLED; > + goto put_power; > > - if (!intel_dp_probe_mst(intel_dp)) { > - drm_modeset_lock(&dev->mode_config.connection_mutex, > NULL); > - intel_dp_check_link_status(intel_dp); > - drm_modeset_unlock(&dev > ->mode_config.connection_mutex); > - goto mst_fail; > - } > } else { > if (intel_dp->is_mst) { > - if (intel_dp_check_mst_status(intel_dp) == -EINVAL) > - goto mst_fail; > + if (intel_dp_check_mst_status(intel_dp) == -EINVAL) { > + /* > + * 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); > + goto put_power; > + } > } > > if (!intel_dp->is_mst) { > @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > > 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) { > - 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: > intel_display_power_put(dev_priv, power_domain); >
On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote: > Current DP detection has DPCD operations split across > intel_dp_hpd_pulse and intel_dp_detect which contains > duplicates as well. Also intel_dp_detect is called > during modes enumeration as well which will result > in multiple dpcd operations. So this patch tries > to solve both these by bringing all DPCD operations > in one single function and make intel_dp_detect > use existing values instead of repeating same steps. > > v2: Pulled in a hunk from last patch of the series to > this patch. (Ander) > v3: Added MST hotplug handling. (Ander) > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++--------------- > - > 1 file changed, 44 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 8969ff9..82ee18d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c [...] > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > return connector_status_disconnected; > } > > - intel_dp_long_pulse(intel_dp->attached_connector); > + if (force) > + intel_dp_long_pulse(intel_dp->attached_connector); I didn't notice this at first, but force is not the right thing to check for here. It is basically intended to avoid doing load detection (see intel_get_load_detect_pipe()) on automated polling. But we still have to try detection here when force = false, otherwise this will cause a regression. If you plug in a DP device while suspended, that device won't get detected, since we don't get an HPD for it. Previously, the call do intel_dp_detect() with force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) would cause a full detection. To avoid the repeated DPCD operations, I think we need a more explicit mechanism to signal that we already handled the long pulse via the HPD handler. In intel_dp_hpd_pulse() we could set a flag that tells we just handled a long pulse for the given port. The call to intel_dp_long_pulse() in intel_dp_detect() would then be dependent on that flag. For that reason I have to retract my R-b from this patch. Ander > if (intel_connector->detect_edid) > return connector_status_connected; > @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > /* indicate that we need to restart link training */ > intel_dp->train_set_valid = false; > > - if (!intel_digital_port_connected(dev_priv, intel_dig_port)) > - goto mst_fail; > - > - if (!intel_dp_get_dpcd(intel_dp)) { > - goto mst_fail; > - } > - > - intel_dp_probe_oui(intel_dp); > + intel_dp_long_pulse(intel_dp->attached_connector); > + if (intel_dp->is_mst) > + ret = IRQ_HANDLED; > + goto put_power; > > - if (!intel_dp_probe_mst(intel_dp)) { > - drm_modeset_lock(&dev->mode_config.connection_mutex, > NULL); > - intel_dp_check_link_status(intel_dp); > - drm_modeset_unlock(&dev > ->mode_config.connection_mutex); > - goto mst_fail; > - } > } else { > if (intel_dp->is_mst) { > - if (intel_dp_check_mst_status(intel_dp) == -EINVAL) > - goto mst_fail; > + if (intel_dp_check_mst_status(intel_dp) == -EINVAL) { > + /* > + * 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); > + goto put_power; > + } > } > > if (!intel_dp->is_mst) { > @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > > 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) { > - 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: > intel_display_power_put(dev_priv, power_domain); >
On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote: > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote: >> Current DP detection has DPCD operations split across >> intel_dp_hpd_pulse and intel_dp_detect which contains >> duplicates as well. Also intel_dp_detect is called >> during modes enumeration as well which will result >> in multiple dpcd operations. So this patch tries >> to solve both these by bringing all DPCD operations >> in one single function and make intel_dp_detect >> use existing values instead of repeating same steps. >> >> v2: Pulled in a hunk from last patch of the series to >> this patch. (Ander) >> v3: Added MST hotplug handling. (Ander) >> >> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> >> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> >> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++--------------- >> - >> 1 file changed, 44 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 8969ff9..82ee18d 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c > [...] > >> @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, bool >> force) >> return connector_status_disconnected; >> } >> >> - intel_dp_long_pulse(intel_dp->attached_connector); >> + if (force) >> + intel_dp_long_pulse(intel_dp->attached_connector); > I didn't notice this at first, but force is not the right thing to check for > here. It is basically intended to avoid doing load detection (see > intel_get_load_detect_pipe()) on automated polling. But we still have to try > detection here when force = false, otherwise this will cause a regression. > > If you plug in a DP device while suspended, that device won't get detected, > since we don't get an HPD for it. Previously, the call do intel_dp_detect() with > force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) would > cause a full detection. > > To avoid the repeated DPCD operations, I think we need a more explicit mechanism > to signal that we already handled the long pulse via the HPD handler. In > intel_dp_hpd_pulse() we could set a flag that tells we just handled a long pulse > for the given port. The call to intel_dp_long_pulse() in intel_dp_detect() would > then be dependent on that flag. > > For that reason I have to retract my R-b from this patch. > > Ander Call to intel_dp_detect() from get_modes is with force set to true while from resume the call is with force set to false.. It should be in the opposite manner as get_modes should not require full detection whereas resume should. So, this needs to be cleaned up there. After merge of these patches, will look into cleaning up that part of the code. Moreover, intel_dp_detect() will be called from drm_helper_hpd_irq_event() in polling scenarios only (when DRM_CONNECTOR_POLL_HPD flag is set in connector->polled). So, seems like this code here, doesn't really create a regression for realtime scenarios. > >> if (intel_connector->detect_edid) >> return connector_status_connected; >> @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port >> *intel_dig_port, bool long_hpd) >> /* indicate that we need to restart link training */ >> intel_dp->train_set_valid = false; >> >> - if (!intel_digital_port_connected(dev_priv, intel_dig_port)) >> - goto mst_fail; >> - >> - if (!intel_dp_get_dpcd(intel_dp)) { >> - goto mst_fail; >> - } >> - >> - intel_dp_probe_oui(intel_dp); >> + intel_dp_long_pulse(intel_dp->attached_connector); >> + if (intel_dp->is_mst) >> + ret = IRQ_HANDLED; >> + goto put_power; >> >> - if (!intel_dp_probe_mst(intel_dp)) { >> - drm_modeset_lock(&dev->mode_config.connection_mutex, >> NULL); >> - intel_dp_check_link_status(intel_dp); >> - drm_modeset_unlock(&dev >> ->mode_config.connection_mutex); >> - goto mst_fail; >> - } >> } else { >> if (intel_dp->is_mst) { >> - if (intel_dp_check_mst_status(intel_dp) == -EINVAL) >> - goto mst_fail; >> + if (intel_dp_check_mst_status(intel_dp) == -EINVAL) { >> + /* >> + * 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); >> + goto put_power; >> + } >> } >> >> if (!intel_dp->is_mst) { >> @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port >> *intel_dig_port, bool long_hpd) >> >> 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) { >> - 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: >> intel_display_power_put(dev_priv, power_domain); >>
On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote: > > On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote: > > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote: > > > Current DP detection has DPCD operations split across > > > intel_dp_hpd_pulse and intel_dp_detect which contains > > > duplicates as well. Also intel_dp_detect is called > > > during modes enumeration as well which will result > > > in multiple dpcd operations. So this patch tries > > > to solve both these by bringing all DPCD operations > > > in one single function and make intel_dp_detect > > > use existing values instead of repeating same steps. > > > > > > v2: Pulled in a hunk from last patch of the series to > > > this patch. (Ander) > > > v3: Added MST hotplug handling. (Ander) > > > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++---------- > > > ----- > > > - > > > 1 file changed, 44 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 8969ff9..82ee18d 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > [...] > > > > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, > > > bool > > > force) > > > return connector_status_disconnected; > > > } > > > > > > - intel_dp_long_pulse(intel_dp->attached_connector); > > > + if (force) > > > + intel_dp_long_pulse(intel_dp->attached_connector); > > I didn't notice this at first, but force is not the right thing to check for > > here. It is basically intended to avoid doing load detection (see > > intel_get_load_detect_pipe()) on automated polling. But we still have to try > > detection here when force = false, otherwise this will cause a regression. > > > > If you plug in a DP device while suspended, that device won't get detected, > > since we don't get an HPD for it. Previously, the call do intel_dp_detect() > > with > > force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) would > > cause a full detection. > > > > To avoid the repeated DPCD operations, I think we need a more explicit > > mechanism > > to signal that we already handled the long pulse via the HPD handler. In > > intel_dp_hpd_pulse() we could set a flag that tells we just handled a long > > pulse > > for the given port. The call to intel_dp_long_pulse() in intel_dp_detect() > > would > > then be dependent on that flag. > > > > For that reason I have to retract my R-b from this patch. > > > > Ander > > Call to intel_dp_detect() from get_modes is with force set to true while > from resume the call is with force set to false.. It should be in the > opposite manner as get_modes should not require full detection whereas > resume should. So, this needs to be cleaned up there. After merge of > these patches, will look into cleaning up that part of the code. That really depends on what the force parameter is supposed to mean. The documentation states that "force is set to false whilst polling, true when checking the connector due to a user request". A look through git history shows the parameter was added to reduce time wasted doing load detection (doing a mode set in order to check if there is a device connected) for hardware that needs it (commit 7b334fcb45b7). As far as I can tell, across all the drm drivers, that parameter is only used to avoid doing load detection. Another thing to consider is that the driver may switch to polling if it detects an HPD storm. When the detect calls come from polling, the code in this patch will simply avoid any detection. > Moreover, intel_dp_detect() will be called from > drm_helper_hpd_irq_event() in polling scenarios only (when > DRM_CONNECTOR_POLL_HPD flag is set in connector->polled). So, seems like > this code here, doesn't really create a regression for realtime scenarios. I don't know what you mean by realtime scenarios, but the regression is very real. Using a kernel with your patches applied, suspend while there is no DP monitor attached, attach the monitor while suspended and then wake up. Notice how the connector state doesn't change. You can check the i915_display_info file in debugfs, for instance. Ander > > > > > if (intel_connector->detect_edid) > > > return connector_status_connected; > > > @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port > > > *intel_dig_port, bool long_hpd) > > > /* indicate that we need to restart link training */ > > > intel_dp->train_set_valid = false; > > > > > > - if (!intel_digital_port_connected(dev_priv, > > > intel_dig_port)) > > > - goto mst_fail; > > > - > > > - if (!intel_dp_get_dpcd(intel_dp)) { > > > - goto mst_fail; > > > - } > > > - > > > - intel_dp_probe_oui(intel_dp); > > > + intel_dp_long_pulse(intel_dp->attached_connector); > > > + if (intel_dp->is_mst) > > > + ret = IRQ_HANDLED; > > > + goto put_power; > > > > > > - if (!intel_dp_probe_mst(intel_dp)) { > > > - drm_modeset_lock(&dev > > > ->mode_config.connection_mutex, > > > NULL); > > > - intel_dp_check_link_status(intel_dp); > > > - drm_modeset_unlock(&dev > > > ->mode_config.connection_mutex); > > > - goto mst_fail; > > > - } > > > } else { > > > if (intel_dp->is_mst) { > > > - if (intel_dp_check_mst_status(intel_dp) == > > > -EINVAL) > > > - goto mst_fail; > > > + if (intel_dp_check_mst_status(intel_dp) == > > > -EINVAL) { > > > + /* > > > + * 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); > > > + goto put_power; > > > + } > > > } > > > > > > if (!intel_dp->is_mst) { > > > @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port > > > *intel_dig_port, bool long_hpd) > > > > > > 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) { > > > - 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: > > > intel_display_power_put(dev_priv, power_domain); > > > >
On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote: > On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote: >> On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote: >>> On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote: >>>> Current DP detection has DPCD operations split across >>>> intel_dp_hpd_pulse and intel_dp_detect which contains >>>> duplicates as well. Also intel_dp_detect is called >>>> during modes enumeration as well which will result >>>> in multiple dpcd operations. So this patch tries >>>> to solve both these by bringing all DPCD operations >>>> in one single function and make intel_dp_detect >>>> use existing values instead of repeating same steps. >>>> >>>> v2: Pulled in a hunk from last patch of the series to >>>> this patch. (Ander) >>>> v3: Added MST hotplug handling. (Ander) >>>> >>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> >>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> >>>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++---------- >>>> ----- >>>> - >>>> 1 file changed, 44 insertions(+), 27 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>>> b/drivers/gpu/drm/i915/intel_dp.c >>>> index 8969ff9..82ee18d 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> [...] >>> >>>> @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, >>>> bool >>>> force) >>>> return connector_status_disconnected; >>>> } >>>> >>>> - intel_dp_long_pulse(intel_dp->attached_connector); >>>> + if (force) >>>> + intel_dp_long_pulse(intel_dp->attached_connector); >>> I didn't notice this at first, but force is not the right thing to check for >>> here. It is basically intended to avoid doing load detection (see >>> intel_get_load_detect_pipe()) on automated polling. But we still have to try >>> detection here when force = false, otherwise this will cause a regression. >>> >>> If you plug in a DP device while suspended, that device won't get detected, >>> since we don't get an HPD for it. Previously, the call do intel_dp_detect() >>> with >>> force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) would >>> cause a full detection. >>> >>> To avoid the repeated DPCD operations, I think we need a more explicit >>> mechanism >>> to signal that we already handled the long pulse via the HPD handler. In >>> intel_dp_hpd_pulse() we could set a flag that tells we just handled a long >>> pulse >>> for the given port. The call to intel_dp_long_pulse() in intel_dp_detect() >>> would >>> then be dependent on that flag. >>> >>> For that reason I have to retract my R-b from this patch. >>> >>> Ander >> Call to intel_dp_detect() from get_modes is with force set to true while >> from resume the call is with force set to false.. It should be in the >> opposite manner as get_modes should not require full detection whereas >> resume should. So, this needs to be cleaned up there. After merge of >> these patches, will look into cleaning up that part of the code. > That really depends on what the force parameter is supposed to mean. The > documentation states that "force is set to false whilst polling, true when > checking the connector due to a user request". A look through git history shows > the parameter was added to reduce time wasted doing load detection (doing a mode > set in order to check if there is a device connected) for hardware that needs it > (commit 7b334fcb45b7). > > As far as I can tell, across all the drm drivers, that parameter is only used to > avoid doing load detection. > > Another thing to consider is that the driver may switch to polling if it detects > an HPD storm. When the detect calls come from polling, the code in this patch > will simply avoid any detection. > hmm i think this discussion will prolong for a while :) how about we call intel_dp_long_pulse() always for now. this will be a compromise to not break any of the existing code but will still result in getting a clean detection code, which will can then be improved upon in the next iteration ? i.e post the change it should look like. i.e skip this change alone intel_dp_long_pulse(intel_dp->attached_connector); regards, Sivakumar >> Moreover, intel_dp_detect() will be called from >> drm_helper_hpd_irq_event() in polling scenarios only (when >> DRM_CONNECTOR_POLL_HPD flag is set in connector->polled). So, seems like >> this code here, doesn't really create a regression for realtime scenarios. > I don't know what you mean by realtime scenarios, but the regression is very > real. Using a kernel with your patches applied, suspend while there is no DP > monitor attached, attach the monitor while suspended and then wake up. Notice > how the connector state doesn't change. You can check the i915_display_info file > in debugfs, for instance. > > > Ander > >>> >>>> if (intel_connector->detect_edid) >>>> return connector_status_connected; >>>> @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port >>>> *intel_dig_port, bool long_hpd) >>>> /* indicate that we need to restart link training */ >>>> intel_dp->train_set_valid = false; >>>> >>>> - if (!intel_digital_port_connected(dev_priv, >>>> intel_dig_port)) >>>> - goto mst_fail; >>>> - >>>> - if (!intel_dp_get_dpcd(intel_dp)) { >>>> - goto mst_fail; >>>> - } >>>> - >>>> - intel_dp_probe_oui(intel_dp); >>>> + intel_dp_long_pulse(intel_dp->attached_connector); >>>> + if (intel_dp->is_mst) >>>> + ret = IRQ_HANDLED; >>>> + goto put_power; >>>> >>>> - if (!intel_dp_probe_mst(intel_dp)) { >>>> - drm_modeset_lock(&dev >>>> ->mode_config.connection_mutex, >>>> NULL); >>>> - intel_dp_check_link_status(intel_dp); >>>> - drm_modeset_unlock(&dev >>>> ->mode_config.connection_mutex); >>>> - goto mst_fail; >>>> - } >>>> } else { >>>> if (intel_dp->is_mst) { >>>> - if (intel_dp_check_mst_status(intel_dp) == >>>> -EINVAL) >>>> - goto mst_fail; >>>> + if (intel_dp_check_mst_status(intel_dp) == >>>> -EINVAL) { >>>> + /* >>>> + * 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); >>>> + goto put_power; >>>> + } >>>> } >>>> >>>> if (!intel_dp->is_mst) { >>>> @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port >>>> *intel_dig_port, bool long_hpd) >>>> >>>> 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) { >>>> - 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: >>>> intel_display_power_put(dev_priv, power_domain); >>>> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2016-02-01 at 11:50 +0530, Thulasimani, Sivakumar wrote: > > On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote: > > On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote: > > > On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote: > > > > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote: > > > > > Current DP detection has DPCD operations split across > > > > > intel_dp_hpd_pulse and intel_dp_detect which contains > > > > > duplicates as well. Also intel_dp_detect is called > > > > > during modes enumeration as well which will result > > > > > in multiple dpcd operations. So this patch tries > > > > > to solve both these by bringing all DPCD operations > > > > > in one single function and make intel_dp_detect > > > > > use existing values instead of repeating same steps. > > > > > > > > > > v2: Pulled in a hunk from last patch of the series to > > > > > this patch. (Ander) > > > > > v3: Added MST hotplug handling. (Ander) > > > > > > > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> > > > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > > > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++----- > > > > > ----- > > > > > ----- > > > > > - > > > > > 1 file changed, 44 insertions(+), 27 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > index 8969ff9..82ee18d 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > [...] > > > > > > > > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, > > > > > bool > > > > > force) > > > > > return connector_status_disconnected; > > > > > } > > > > > > > > > > - intel_dp_long_pulse(intel_dp->attached_connector); > > > > > + if (force) > > > > > + intel_dp_long_pulse(intel_dp->attached_connector); > > > > I didn't notice this at first, but force is not the right thing to check > > > > for > > > > here. It is basically intended to avoid doing load detection (see > > > > intel_get_load_detect_pipe()) on automated polling. But we still have to > > > > try > > > > detection here when force = false, otherwise this will cause a > > > > regression. > > > > > > > > If you plug in a DP device while suspended, that device won't get > > > > detected, > > > > since we don't get an HPD for it. Previously, the call do > > > > intel_dp_detect() > > > > with > > > > force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) > > > > would > > > > cause a full detection. > > > > > > > > To avoid the repeated DPCD operations, I think we need a more explicit > > > > mechanism > > > > to signal that we already handled the long pulse via the HPD handler. In > > > > intel_dp_hpd_pulse() we could set a flag that tells we just handled a > > > > long > > > > pulse > > > > for the given port. The call to intel_dp_long_pulse() in > > > > intel_dp_detect() > > > > would > > > > then be dependent on that flag. > > > > > > > > For that reason I have to retract my R-b from this patch. > > > > > > > > Ander > > > Call to intel_dp_detect() from get_modes is with force set to true while > > > from resume the call is with force set to false.. It should be in the > > > opposite manner as get_modes should not require full detection whereas > > > resume should. So, this needs to be cleaned up there. After merge of > > > these patches, will look into cleaning up that part of the code. > > That really depends on what the force parameter is supposed to mean. The > > documentation states that "force is set to false whilst polling, true when > > checking the connector due to a user request". A look through git history > > shows > > the parameter was added to reduce time wasted doing load detection (doing a > > mode > > set in order to check if there is a device connected) for hardware that > > needs it > > (commit 7b334fcb45b7). > > > > As far as I can tell, across all the drm drivers, that parameter is only > > used to > > avoid doing load detection. > > > > Another thing to consider is that the driver may switch to polling if it > > detects > > an HPD storm. When the detect calls come from polling, the code in this > > patch > > will simply avoid any detection. > > > hmm i think this discussion will prolong for a while :) > how about we call intel_dp_long_pulse() always for now. > this will be a compromise to not break any of the existing code > but will still result in getting a clean detection code, which > will can then be improved upon in the next iteration ? > i.e post the change it should look like. i.e skip this change alone > > intel_dp_long_pulse(intel_dp->attached_connector); Sounds good. The code gets cleaner and there is no regression in terms of repeated DPCD operations. Ander > > > regards, > Sivakumar > > > Moreover, intel_dp_detect() will be called from > > > drm_helper_hpd_irq_event() in polling scenarios only (when > > > DRM_CONNECTOR_POLL_HPD flag is set in connector->polled). So, seems like > > > this code here, doesn't really create a regression for realtime scenarios. > > I don't know what you mean by realtime scenarios, but the regression is very > > real. Using a kernel with your patches applied, suspend while there is no DP > > monitor attached, attach the monitor while suspended and then wake up. > > Notice > > how the connector state doesn't change. You can check the i915_display_info > > file > > in debugfs, for instance. > > > > > > Ander > > > > > > > > > > > if (intel_connector->detect_edid) > > > > > return connector_status_connected; > > > > > @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port > > > > > *intel_dig_port, bool long_hpd) > > > > > /* indicate that we need to restart link training > > > > > */ > > > > > intel_dp->train_set_valid = false; > > > > > > > > > > - if (!intel_digital_port_connected(dev_priv, > > > > > intel_dig_port)) > > > > > - goto mst_fail; > > > > > - > > > > > - if (!intel_dp_get_dpcd(intel_dp)) { > > > > > - goto mst_fail; > > > > > - } > > > > > - > > > > > - intel_dp_probe_oui(intel_dp); > > > > > + intel_dp_long_pulse(intel_dp->attached_connector); > > > > > + if (intel_dp->is_mst) > > > > > + ret = IRQ_HANDLED; > > > > > + goto put_power; > > > > > > > > > > - if (!intel_dp_probe_mst(intel_dp)) { > > > > > - drm_modeset_lock(&dev > > > > > ->mode_config.connection_mutex, > > > > > NULL); > > > > > - intel_dp_check_link_status(intel_dp); > > > > > - drm_modeset_unlock(&dev > > > > > ->mode_config.connection_mutex); > > > > > - goto mst_fail; > > > > > - } > > > > > } else { > > > > > if (intel_dp->is_mst) { > > > > > - if (intel_dp_check_mst_status(intel_dp) == > > > > > -EINVAL) > > > > > - goto mst_fail; > > > > > + if (intel_dp_check_mst_status(intel_dp) == > > > > > -EINVAL) { > > > > > + /* > > > > > + * 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(&inte > > > > > l_dp > > > > > ->mst_mgr, > > > > > + intel > > > > > _dp > > > > > ->is_mst); > > > > > + goto put_power; > > > > > + } > > > > > } > > > > > > > > > > if (!intel_dp->is_mst) { > > > > > @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port > > > > > *intel_dig_port, bool long_hpd) > > > > > > > > > > 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) { > > > > > - 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: > > > > > intel_display_power_put(dev_priv, power_domain); > > > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Feb 01, 2016 at 11:13:08AM +0200, Ander Conselvan De Oliveira wrote: > On Mon, 2016-02-01 at 11:50 +0530, Thulasimani, Sivakumar wrote: > > > > On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote: > > > On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote: > > > > On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote: > > > > > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote: > > > > > > Current DP detection has DPCD operations split across > > > > > > intel_dp_hpd_pulse and intel_dp_detect which contains > > > > > > duplicates as well. Also intel_dp_detect is called > > > > > > during modes enumeration as well which will result > > > > > > in multiple dpcd operations. So this patch tries > > > > > > to solve both these by bringing all DPCD operations > > > > > > in one single function and make intel_dp_detect > > > > > > use existing values instead of repeating same steps. > > > > > > > > > > > > v2: Pulled in a hunk from last patch of the series to > > > > > > this patch. (Ander) > > > > > > v3: Added MST hotplug handling. (Ander) > > > > > > > > > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> > > > > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > > > > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++----- > > > > > > ----- > > > > > > ----- > > > > > > - > > > > > > 1 file changed, 44 insertions(+), 27 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > > index 8969ff9..82ee18d 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > [...] > > > > > > > > > > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, > > > > > > bool > > > > > > force) > > > > > > return connector_status_disconnected; > > > > > > } > > > > > > > > > > > > - intel_dp_long_pulse(intel_dp->attached_connector); > > > > > > + if (force) > > > > > > + intel_dp_long_pulse(intel_dp->attached_connector); > > > > > I didn't notice this at first, but force is not the right thing to check > > > > > for > > > > > here. It is basically intended to avoid doing load detection (see > > > > > intel_get_load_detect_pipe()) on automated polling. But we still have to > > > > > try > > > > > detection here when force = false, otherwise this will cause a > > > > > regression. > > > > > > > > > > If you plug in a DP device while suspended, that device won't get > > > > > detected, > > > > > since we don't get an HPD for it. Previously, the call do > > > > > intel_dp_detect() > > > > > with > > > > > force = false from intel_drm_resume() (via drm_helper_hpd_irq_event()) > > > > > would > > > > > cause a full detection. > > > > > > > > > > To avoid the repeated DPCD operations, I think we need a more explicit > > > > > mechanism > > > > > to signal that we already handled the long pulse via the HPD handler. In > > > > > intel_dp_hpd_pulse() we could set a flag that tells we just handled a > > > > > long > > > > > pulse > > > > > for the given port. The call to intel_dp_long_pulse() in > > > > > intel_dp_detect() > > > > > would > > > > > then be dependent on that flag. > > > > > > > > > > For that reason I have to retract my R-b from this patch. > > > > > > > > > > Ander > > > > Call to intel_dp_detect() from get_modes is with force set to true while > > > > from resume the call is with force set to false.. It should be in the > > > > opposite manner as get_modes should not require full detection whereas > > > > resume should. So, this needs to be cleaned up there. After merge of > > > > these patches, will look into cleaning up that part of the code. > > > That really depends on what the force parameter is supposed to mean. The > > > documentation states that "force is set to false whilst polling, true when > > > checking the connector due to a user request". A look through git history > > > shows > > > the parameter was added to reduce time wasted doing load detection (doing a > > > mode > > > set in order to check if there is a device connected) for hardware that > > > needs it > > > (commit 7b334fcb45b7). > > > > > > As far as I can tell, across all the drm drivers, that parameter is only > > > used to > > > avoid doing load detection. > > > > > > Another thing to consider is that the driver may switch to polling if it > > > detects > > > an HPD storm. When the detect calls come from polling, the code in this > > > patch > > > will simply avoid any detection. > > > > > hmm i think this discussion will prolong for a while :) > > how about we call intel_dp_long_pulse() always for now. > > this will be a compromise to not break any of the existing code > > but will still result in getting a clean detection code, which > > will can then be improved upon in the next iteration ? > > i.e post the change it should look like. i.e skip this change alone > > > > intel_dp_long_pulse(intel_dp->attached_connector); > > Sounds good. The code gets cleaner and there is no regression in terms of > repeated DPCD operations. So this conversation seems to have had little bearing on reality, especially in terms of how intel_dp_detect() is supposed to behave. This patch is causing WARNs as it presumed that intel_dp_detect() is only called after a modeset. Should we send a revert to stable? -Chris
On Thu, 2016-11-17 at 22:01 +0000, Chris Wilson wrote: > On Mon, Feb 01, 2016 at 11:13:08AM +0200, Ander Conselvan De Oliveira wrote: > > > > On Mon, 2016-02-01 at 11:50 +0530, Thulasimani, Sivakumar wrote: > > > > > > > > > On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote: > > > > > > > > On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote: > > > > > > > > > > On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira > > > > > wrote: > > > > > > > > > > > > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote: > > > > > > > > > > > > > > Current DP detection has DPCD operations split across > > > > > > > intel_dp_hpd_pulse and intel_dp_detect which contains > > > > > > > duplicates as well. Also intel_dp_detect is called > > > > > > > during modes enumeration as well which will result > > > > > > > in multiple dpcd operations. So this patch tries > > > > > > > to solve both these by bringing all DPCD operations > > > > > > > in one single function and make intel_dp_detect > > > > > > > use existing values instead of repeating same steps. > > > > > > > > > > > > > > v2: Pulled in a hunk from last patch of the series to > > > > > > > this patch. (Ander) > > > > > > > v3: Added MST hotplug handling. (Ander) > > > > > > > > > > > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> > > > > > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel. > > > > > > > com> > > > > > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel. > > > > > > > com> > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++- > > > > > > > ---- > > > > > > > ----- > > > > > > > ----- > > > > > > > - > > > > > > > 1 file changed, 44 insertions(+), 27 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > > > index 8969ff9..82ee18d 100644 > > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > > [...] > > > > > > > > > > > > > > > > > > > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector > > > > > > > *connector, > > > > > > > bool > > > > > > > force) > > > > > > > return connector_status_disconnected; > > > > > > > } > > > > > > > > > > > > > > - intel_dp_long_pulse(intel_dp->attached_connector); > > > > > > > + if (force) > > > > > > > + intel_dp_long_pulse(intel_dp- > > > > > > > >attached_connector); > > > > > > I didn't notice this at first, but force is not the right thing to > > > > > > check > > > > > > for > > > > > > here. It is basically intended to avoid doing load detection (see > > > > > > intel_get_load_detect_pipe()) on automated polling. But we still > > > > > > have to > > > > > > try > > > > > > detection here when force = false, otherwise this will cause a > > > > > > regression. > > > > > > > > > > > > If you plug in a DP device while suspended, that device won't get > > > > > > detected, > > > > > > since we don't get an HPD for it. Previously, the call do > > > > > > intel_dp_detect() > > > > > > with > > > > > > force = false from intel_drm_resume() (via > > > > > > drm_helper_hpd_irq_event()) > > > > > > would > > > > > > cause a full detection. > > > > > > > > > > > > To avoid the repeated DPCD operations, I think we need a more > > > > > > explicit > > > > > > mechanism > > > > > > to signal that we already handled the long pulse via the HPD > > > > > > handler. In > > > > > > intel_dp_hpd_pulse() we could set a flag that tells we just handled > > > > > > a > > > > > > long > > > > > > pulse > > > > > > for the given port. The call to intel_dp_long_pulse() in > > > > > > intel_dp_detect() > > > > > > would > > > > > > then be dependent on that flag. > > > > > > > > > > > > For that reason I have to retract my R-b from this patch. > > > > > > > > > > > > Ander > > > > > Call to intel_dp_detect() from get_modes is with force set to true > > > > > while > > > > > from resume the call is with force set to false.. It should be in the > > > > > opposite manner as get_modes should not require full detection whereas > > > > > resume should. So, this needs to be cleaned up there. After merge of > > > > > these patches, will look into cleaning up that part of the code. > > > > That really depends on what the force parameter is supposed to mean. The > > > > documentation states that "force is set to false whilst polling, true > > > > when > > > > checking the connector due to a user request". A look through git > > > > history > > > > shows > > > > the parameter was added to reduce time wasted doing load detection > > > > (doing a > > > > mode > > > > set in order to check if there is a device connected) for hardware that > > > > needs it > > > > (commit 7b334fcb45b7). > > > > > > > > As far as I can tell, across all the drm drivers, that parameter is only > > > > used to > > > > avoid doing load detection. > > > > > > > > Another thing to consider is that the driver may switch to polling if it > > > > detects > > > > an HPD storm. When the detect calls come from polling, the code in this > > > > patch > > > > will simply avoid any detection. > > > > > > > hmm i think this discussion will prolong for a while :) > > > how about we call intel_dp_long_pulse() always for now. > > > this will be a compromise to not break any of the existing code > > > but will still result in getting a clean detection code, which > > > will can then be improved upon in the next iteration ? > > > i.e post the change it should look like. i.e skip this change alone > > > > > > intel_dp_long_pulse(intel_dp->attached_connector); > > Sounds good. The code gets cleaner and there is no regression in terms of > > repeated DPCD operations. > So this conversation seems to have had little bearing on reality, > especially in terms of how intel_dp_detect() is supposed to behave. This > patch is causing WARNs as it presumed that intel_dp_detect() is only > called after a modeset. > > Should we send a revert to stable? Ville's 27d4efc5591a ("drm/i915: Move long hpd handling into the hotplug work") and 16c83fad79ca ("drm/i915: Allow DP to work w/o EDID") were sent to stable and are in 4.8.6. Are the WARNs happening with those patches too? Ander
On Fri, Nov 18, 2016 at 11:41:37AM +0200, Ander Conselvan De Oliveira wrote: > On Thu, 2016-11-17 at 22:01 +0000, Chris Wilson wrote: > > On Mon, Feb 01, 2016 at 11:13:08AM +0200, Ander Conselvan De Oliveira wrote: > > > > > > On Mon, 2016-02-01 at 11:50 +0530, Thulasimani, Sivakumar wrote: > > > > > > > > > > > > On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote: > > > > > > > > > > On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote: > > > > > > > > > > > > On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira > > > > > > wrote: > > > > > > > > > > > > > > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote: > > > > > > > > > > > > > > > > Current DP detection has DPCD operations split across > > > > > > > > intel_dp_hpd_pulse and intel_dp_detect which contains > > > > > > > > duplicates as well. Also intel_dp_detect is called > > > > > > > > during modes enumeration as well which will result > > > > > > > > in multiple dpcd operations. So this patch tries > > > > > > > > to solve both these by bringing all DPCD operations > > > > > > > > in one single function and make intel_dp_detect > > > > > > > > use existing values instead of repeating same steps. > > > > > > > > > > > > > > > > v2: Pulled in a hunk from last patch of the series to > > > > > > > > this patch. (Ander) > > > > > > > > v3: Added MST hotplug handling. (Ander) > > > > > > > > > > > > > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> > > > > > > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel. > > > > > > > > com> > > > > > > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel. > > > > > > > > com> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++- > > > > > > > > ---- > > > > > > > > ----- > > > > > > > > ----- > > > > > > > > - > > > > > > > > 1 file changed, 44 insertions(+), 27 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > > > > index 8969ff9..82ee18d 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector > > > > > > > > *connector, > > > > > > > > bool > > > > > > > > force) > > > > > > > > return connector_status_disconnected; > > > > > > > > } > > > > > > > > > > > > > > > > - intel_dp_long_pulse(intel_dp->attached_connector); > > > > > > > > + if (force) > > > > > > > > + intel_dp_long_pulse(intel_dp- > > > > > > > > >attached_connector); > > > > > > > I didn't notice this at first, but force is not the right thing to > > > > > > > check > > > > > > > for > > > > > > > here. It is basically intended to avoid doing load detection (see > > > > > > > intel_get_load_detect_pipe()) on automated polling. But we still > > > > > > > have to > > > > > > > try > > > > > > > detection here when force = false, otherwise this will cause a > > > > > > > regression. > > > > > > > > > > > > > > If you plug in a DP device while suspended, that device won't get > > > > > > > detected, > > > > > > > since we don't get an HPD for it. Previously, the call do > > > > > > > intel_dp_detect() > > > > > > > with > > > > > > > force = false from intel_drm_resume() (via > > > > > > > drm_helper_hpd_irq_event()) > > > > > > > would > > > > > > > cause a full detection. > > > > > > > > > > > > > > To avoid the repeated DPCD operations, I think we need a more > > > > > > > explicit > > > > > > > mechanism > > > > > > > to signal that we already handled the long pulse via the HPD > > > > > > > handler. In > > > > > > > intel_dp_hpd_pulse() we could set a flag that tells we just handled > > > > > > > a > > > > > > > long > > > > > > > pulse > > > > > > > for the given port. The call to intel_dp_long_pulse() in > > > > > > > intel_dp_detect() > > > > > > > would > > > > > > > then be dependent on that flag. > > > > > > > > > > > > > > For that reason I have to retract my R-b from this patch. > > > > > > > > > > > > > > Ander > > > > > > Call to intel_dp_detect() from get_modes is with force set to true > > > > > > while > > > > > > from resume the call is with force set to false.. It should be in the > > > > > > opposite manner as get_modes should not require full detection whereas > > > > > > resume should. So, this needs to be cleaned up there. After merge of > > > > > > these patches, will look into cleaning up that part of the code. > > > > > That really depends on what the force parameter is supposed to mean. The > > > > > documentation states that "force is set to false whilst polling, true > > > > > when > > > > > checking the connector due to a user request". A look through git > > > > > history > > > > > shows > > > > > the parameter was added to reduce time wasted doing load detection > > > > > (doing a > > > > > mode > > > > > set in order to check if there is a device connected) for hardware that > > > > > needs it > > > > > (commit 7b334fcb45b7). > > > > > > > > > > As far as I can tell, across all the drm drivers, that parameter is only > > > > > used to > > > > > avoid doing load detection. > > > > > > > > > > Another thing to consider is that the driver may switch to polling if it > > > > > detects > > > > > an HPD storm. When the detect calls come from polling, the code in this > > > > > patch > > > > > will simply avoid any detection. > > > > > > > > > hmm i think this discussion will prolong for a while :) > > > > how about we call intel_dp_long_pulse() always for now. > > > > this will be a compromise to not break any of the existing code > > > > but will still result in getting a clean detection code, which > > > > will can then be improved upon in the next iteration ? > > > > i.e post the change it should look like. i.e skip this change alone > > > > > > > > intel_dp_long_pulse(intel_dp->attached_connector); > > > Sounds good. The code gets cleaner and there is no regression in terms of > > > repeated DPCD operations. > > So this conversation seems to have had little bearing on reality, > > especially in terms of how intel_dp_detect() is supposed to behave. This > > patch is causing WARNs as it presumed that intel_dp_detect() is only > > called after a modeset. > > > > Should we send a revert to stable? > > Ville's 27d4efc5591a ("drm/i915: Move long hpd handling into the hotplug work") > and 16c83fad79ca ("drm/i915: Allow DP to work w/o EDID") were sent to stable and > are in 4.8.6. Are the WARNs happening with those patches too? Yes. The WARNs are still happening in -nightly, they only take calling GETCONNECTOR twice on a connected monitor before setting a mode. -Chris
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 8969ff9..82ee18d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4638,8 +4638,19 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) intel_dp_probe_oui(intel_dp); ret = intel_dp_probe_mst(intel_dp); - if (ret) + if (ret) { + goto out; + } else if (connector->status == connector_status_connected) { + /* + * If display was connected already and is still connected + * check links status, there has been known issues of + * link loss triggerring long pulse!!!! + */ + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); + intel_dp_check_link_status(intel_dp); + drm_modeset_unlock(&dev->mode_config.connection_mutex); goto out; + } /* * Clearing NACK and defer counts to get their exact values @@ -4668,8 +4679,21 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) } out: - if (status != connector_status_connected) + if (status != connector_status_connected) { intel_dp_unset_edid(intel_dp); + /* + * If we were in MST mode, and device is not there, + * get out of MST mode + */ + if (intel_dp->is_mst) { + 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); + } + } + intel_display_power_put(to_i915(dev), power_domain); return; } @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector, bool force) return connector_status_disconnected; } - intel_dp_long_pulse(intel_dp->attached_connector); + if (force) + intel_dp_long_pulse(intel_dp->attached_connector); if (intel_connector->detect_edid) return connector_status_connected; @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) /* indicate that we need to restart link training */ intel_dp->train_set_valid = false; - if (!intel_digital_port_connected(dev_priv, intel_dig_port)) - goto mst_fail; - - if (!intel_dp_get_dpcd(intel_dp)) { - goto mst_fail; - } - - intel_dp_probe_oui(intel_dp); + intel_dp_long_pulse(intel_dp->attached_connector); + if (intel_dp->is_mst) + ret = IRQ_HANDLED; + goto put_power; - if (!intel_dp_probe_mst(intel_dp)) { - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); - drm_modeset_unlock(&dev->mode_config.connection_mutex); - goto mst_fail; - } } else { if (intel_dp->is_mst) { - if (intel_dp_check_mst_status(intel_dp) == -EINVAL) - goto mst_fail; + if (intel_dp_check_mst_status(intel_dp) == -EINVAL) { + /* + * 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); + goto put_power; + } } if (!intel_dp->is_mst) { @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) 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) { - 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: intel_display_power_put(dev_priv, power_domain);
Current DP detection has DPCD operations split across intel_dp_hpd_pulse and intel_dp_detect which contains duplicates as well. Also intel_dp_detect is called during modes enumeration as well which will result in multiple dpcd operations. So this patch tries to solve both these by bringing all DPCD operations in one single function and make intel_dp_detect use existing values instead of repeating same steps. v2: Pulled in a hunk from last patch of the series to this patch. (Ander) v3: Added MST hotplug handling. (Ander) Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 27 deletions(-)