| Message ID | 1446812886-32156-3-git-send-email-shubhangi.shrivastava@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2015-11-06 at 17:58 +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. > > 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 | 19 ++++--------------- > 1 file changed, 4 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index a0fe827..4e74cd6 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4881,7 +4881,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; > @@ -5211,21 +5212,9 @@ 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; > + intel_dp_long_pulse(intel_dp->attached_connector); > + goto put_power; > > - if (!intel_dp_get_dpcd(intel_dp)) { > - goto mst_fail; > - } So we don't call this for eDP anymore on long pulse, which I assume is harmless since the bits we are reading from DPCD shouldn't change? > - > - intel_dp_probe_oui(intel_dp); > - > - 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; > - } Hmm, so this is where that hunk from patch 1 I said should be a separate patch comes from. Looks like in belongs to this patch. > } else { > if (intel_dp->is_mst) { > if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
On Mon, 2015-11-16 at 16:33 +0200, Ander Conselvan De Oliveira wrote: > On Fri, 2015-11-06 at 17:58 +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. > > > > 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 | 19 ++++--------------- > > 1 file changed, 4 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index a0fe827..4e74cd6 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4881,7 +4881,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; > > @@ -5211,21 +5212,9 @@ 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; > > + intel_dp_long_pulse(intel_dp->attached_connector); > > + goto put_power; This skips the line that sets ret to IRQ_HANDLED, which will cause the hotplug code to "fall back to old school hpd". Ander > > > > - if (!intel_dp_get_dpcd(intel_dp)) { > > - goto mst_fail; > > - } > > So we don't call this for eDP anymore on long pulse, which I assume is > harmless > since the bits we are reading from DPCD shouldn't change? > > > - > > - intel_dp_probe_oui(intel_dp); > > - > > - 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; > > - } > > Hmm, so this is where that hunk from patch 1 I said should be a separate patch > comes from. Looks like in belongs to this patch. > > > } else { > > if (intel_dp->is_mst) { > > if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
On Monday 16 November 2015 08:03 PM, Ander Conselvan De Oliveira wrote: > On Fri, 2015-11-06 at 17:58 +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. >> >> 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 | 19 ++++--------------- >> 1 file changed, 4 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index a0fe827..4e74cd6 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4881,7 +4881,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; >> @@ -5211,21 +5212,9 @@ 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; >> + intel_dp_long_pulse(intel_dp->attached_connector); >> + goto put_power; >> >> - if (!intel_dp_get_dpcd(intel_dp)) { >> - goto mst_fail; >> - } > So we don't call this for eDP anymore on long pulse, which I assume is harmless > since the bits we are reading from DPCD shouldn't change? > >> - >> - intel_dp_probe_oui(intel_dp); >> - >> - 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; >> - } > Hmm, so this is where that hunk from patch 1 I said should be a separate patch > comes from. Looks like in belongs to this patch. Pulled in that hunk from patch 1 to this patch. > >> } else { >> if (intel_dp->is_mst) { >> if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
On Monday 16 November 2015 08:16 PM, Ander Conselvan De Oliveira wrote: > On Mon, 2015-11-16 at 16:33 +0200, Ander Conselvan De Oliveira wrote: >> On Fri, 2015-11-06 at 17:58 +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. >>> >>> 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 | 19 ++++--------------- >>> 1 file changed, 4 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>> b/drivers/gpu/drm/i915/intel_dp.c >>> index a0fe827..4e74cd6 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -4881,7 +4881,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; >>> @@ -5211,21 +5212,9 @@ 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; >>> + intel_dp_long_pulse(intel_dp->attached_connector); >>> + goto put_power; > This skips the line that sets ret to IRQ_HANDLED, which will cause the hotplug > code to "fall back to old school hpd". > > Ander Notification of hotplug to user mode goes from i915_hotplug_work_func. Returning IRQ_NONE from here will notify user mode about hotplug. > >>> >>> - if (!intel_dp_get_dpcd(intel_dp)) { >>> - goto mst_fail; >>> - } >> So we don't call this for eDP anymore on long pulse, which I assume is >> harmless >> since the bits we are reading from DPCD shouldn't change? >> >>> - >>> - intel_dp_probe_oui(intel_dp); >>> - >>> - 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; >>> - } >> Hmm, so this is where that hunk from patch 1 I said should be a separate patch >> comes from. Looks like in belongs to this patch. >> >>> } else { >>> if (intel_dp->is_mst) { >>> if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
On Tue, 2015-11-17 at 12:23 +0530, Shubhangi Shrivastava wrote: > > On Monday 16 November 2015 08:16 PM, Ander Conselvan De Oliveira wrote: > > On Mon, 2015-11-16 at 16:33 +0200, Ander Conselvan De Oliveira wrote: > > > On Fri, 2015-11-06 at 17:58 +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. > > > > > > > > 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 | 19 ++++--------------- > > > > 1 file changed, 4 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index a0fe827..4e74cd6 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -4881,7 +4881,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; > > > > @@ -5211,21 +5212,9 @@ 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; > > > > + intel_dp_long_pulse(intel_dp->attached_connector); > > > > + goto put_power; > > This skips the line that sets ret to IRQ_HANDLED, which will cause the > > hotplug > > code to "fall back to old school hpd". > > > > Ander > Notification of hotplug to user mode goes from i915_hotplug_work_func. > Returning IRQ_NONE from here will notify user mode about hotplug. So if I understand correctly, previously if the device was connected, we succeeded in getting the dpcd and the device was mst, there was no hotplug event being sent. With the change above the hotplug event is generated unconditionally, and we never do the error handling in mst_fail for the long pul se. Ander > > > > > > > > > > - if (!intel_dp_get_dpcd(intel_dp)) { > > > > - goto mst_fail; > > > > - } > > > So we don't call this for eDP anymore on long pulse, which I assume is > > > harmless > > > since the bits we are reading from DPCD shouldn't change? > > > > > > > - > > > > - intel_dp_probe_oui(intel_dp); > > > > - > > > > - 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; > > > > - } > > > Hmm, so this is where that hunk from patch 1 I said should be a separate > > > patch > > > comes from. Looks like in belongs to this patch. > > > > > > > } else { > > > > if (intel_dp->is_mst) { > > > > if (intel_dp_check_mst_status(intel_dp) == > > > > -EINVAL) >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a0fe827..4e74cd6 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4881,7 +4881,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; @@ -5211,21 +5212,9 @@ 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; + intel_dp_long_pulse(intel_dp->attached_connector); + goto put_power; - if (!intel_dp_get_dpcd(intel_dp)) { - goto mst_fail; - } - - intel_dp_probe_oui(intel_dp); - - 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)