| Message ID | 1451998226-21433-2-git-send-email-shubhangi.shrivastava@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: > intel_dp_detect() is called for not just detection but > during modes enumeration as well. Repeating the whole > sequence during each of these calls is wasteful and > time consuming. > This patch moves probing for panel, DPCD read etc done in > intel_dp_detect() to a new function intel_dp_long_pulse(). > Note that the behavior of intel_dp_detect() is changed to > report connected or disconnected depending on whether the > EDID is available or not. > This change will be required by further patches in the series > to avoid performing duplicated DPCD operations on hotplug. > > v2: Moved a hunk to next patch of the series. > Moved intel_dp_unset_edid to out. (Ander) > v3: Rephrased commit message and intel_dp_unset_dp() is called > within intel_dp_set_dp() to free the previous EDID. (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 | 56 +++++++++++++++++++++++++--------------- > - > 1 file changed, 35 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 796e3d3..e3b4208 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, > bool sync); > static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp); > static void vlv_steal_power_sequencer(struct drm_device *dev, > enum pipe pipe); > +static void intel_dp_unset_edid(struct intel_dp *intel_dp); > > static unsigned int intel_dp_unused_lane_mask(int lane_count) > { > @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp) > struct intel_connector *intel_connector = intel_dp > ->attached_connector; > struct edid *edid; > > + intel_dp_unset_edid(intel_dp); > edid = intel_dp_get_edid(intel_dp); > intel_connector->detect_edid = edid; > > @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > intel_dp->has_audio = false; > } > > -static enum drm_connector_status > -intel_dp_detect(struct drm_connector *connector, bool force) > +static void > +intel_dp_long_pulse(struct intel_connector *intel_connector) > { > + struct drm_connector *connector = &intel_connector->base; > struct intel_dp *intel_dp = intel_attached_dp(connector); > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > struct intel_encoder *intel_encoder = &intel_dig_port->base; > @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > bool ret; > u8 sink_irq_vector; > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > - connector->base.id, connector->name); > - intel_dp_unset_edid(intel_dp); > - > - if (intel_dp->is_mst) { > - /* MST devices are disconnected from a monitor POV */ > - if (intel_encoder->type != INTEL_OUTPUT_EDP) > - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > - return connector_status_disconnected; > - } > - > power_domain = intel_display_port_aux_power_domain(intel_encoder); > intel_display_power_get(to_i915(dev), power_domain); > > @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > intel_dp_probe_oui(intel_dp); > > ret = intel_dp_probe_mst(intel_dp); > - if (ret) { > - /* if we are in MST mode then this connector > - won't appear connected or have anything with EDID on it */ > - if (intel_encoder->type != INTEL_OUTPUT_EDP) > - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; This deletion is new in this version of the patch. I think we still need the hunk above, otherwise we might not properly update the encoder type when we switch from an HDMI sink connected through a level shifter to an MST sink. Ander > - status = connector_status_disconnected; > + if (ret) > goto out; > - } > > /* > * Clearing NACK and defer counts to get their exact values > @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > } > > out: > + if (status != connector_status_connected) > + intel_dp_unset_edid(intel_dp); > intel_display_power_put(to_i915(dev), power_domain); > - return status; > + return; > +} > + > +static enum drm_connector_status > +intel_dp_detect(struct drm_connector *connector, bool force) > +{ > + struct intel_dp *intel_dp = intel_attached_dp(connector); > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > + struct intel_connector *intel_connector = > to_intel_connector(connector); > + > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > + connector->base.id, connector->name); > + > + if (intel_dp->is_mst) { > + /* MST devices are disconnected from a monitor POV */ > + if (intel_encoder->type != INTEL_OUTPUT_EDP) > + intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > + return connector_status_disconnected; > + } > + > + intel_dp_long_pulse(intel_dp->attached_connector); > + > + if (intel_connector->detect_edid) > + return connector_status_connected; > + else > + return connector_status_disconnected; > } > > static void
On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote: > On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: > > intel_dp_detect() is called for not just detection but > > during modes enumeration as well. Repeating the whole > > sequence during each of these calls is wasteful and > > time consuming. > > This patch moves probing for panel, DPCD read etc done in > > intel_dp_detect() to a new function intel_dp_long_pulse(). > > Note that the behavior of intel_dp_detect() is changed to > > report connected or disconnected depending on whether the > > EDID is available or not. > > This change will be required by further patches in the series > > to avoid performing duplicated DPCD operations on hotplug. > > > > v2: Moved a hunk to next patch of the series. > > Moved intel_dp_unset_edid to out. (Ander) > > v3: Rephrased commit message and intel_dp_unset_dp() is called > > within intel_dp_set_dp() to free the previous EDID. (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 | 56 +++++++++++++++++++++++++------------- > > -- > > - > > 1 file changed, 35 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 796e3d3..e3b4208 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, > > bool sync); > > static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp); > > static void vlv_steal_power_sequencer(struct drm_device *dev, > > enum pipe pipe); > > +static void intel_dp_unset_edid(struct intel_dp *intel_dp); > > > > static unsigned int intel_dp_unused_lane_mask(int lane_count) > > { > > @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp) > > struct intel_connector *intel_connector = intel_dp > > ->attached_connector; > > struct edid *edid; > > > > + intel_dp_unset_edid(intel_dp); > > edid = intel_dp_get_edid(intel_dp); > > intel_connector->detect_edid = edid; > > > > @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > > intel_dp->has_audio = false; > > } > > > > -static enum drm_connector_status > > -intel_dp_detect(struct drm_connector *connector, bool force) > > +static void > > +intel_dp_long_pulse(struct intel_connector *intel_connector) > > { > > + struct drm_connector *connector = &intel_connector->base; > > struct intel_dp *intel_dp = intel_attached_dp(connector); > > struct intel_digital_port *intel_dig_port = > > dp_to_dig_port(intel_dp); > > struct intel_encoder *intel_encoder = &intel_dig_port->base; > > @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, bool > > force) > > bool ret; > > u8 sink_irq_vector; > > > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > - connector->base.id, connector->name); > > - intel_dp_unset_edid(intel_dp); > > - > > - if (intel_dp->is_mst) { > > - /* MST devices are disconnected from a monitor POV */ > > - if (intel_encoder->type != INTEL_OUTPUT_EDP) > > - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > > - return connector_status_disconnected; > > - } > > - > > power_domain = intel_display_port_aux_power_domain(intel_encoder); > > intel_display_power_get(to_i915(dev), power_domain); > > > > @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool > > force) > > intel_dp_probe_oui(intel_dp); > > > > ret = intel_dp_probe_mst(intel_dp); > > - if (ret) { > > - /* if we are in MST mode then this connector > > - won't appear connected or have anything with EDID on it > > */ > > - if (intel_encoder->type != INTEL_OUTPUT_EDP) > > - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > > This deletion is new in this version of the patch. I think we still need the > hunk above, otherwise we might not properly update the encoder type when we > switch from an HDMI sink connected through a level shifter to an MST sink. > > Ander > > > > - status = connector_status_disconnected; > > + if (ret) > > goto out; Also, there is no call to intel_dp_unset_edid() for this case, since the code will reach the label 'out' with status being connected. So in this case the return value of intel_dp_detect() will depend on the stale value of intel_dp->detect_edid. Ander > > - } > > > > /* > > * Clearing NACK and defer counts to get their exact values > > @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool > > force) > > } > > > > out: > > + if (status != connector_status_connected) > > + intel_dp_unset_edid(intel_dp); > > intel_display_power_put(to_i915(dev), power_domain); > > - return status; > > + return; > > +} > > + > > +static enum drm_connector_status > > +intel_dp_detect(struct drm_connector *connector, bool force) > > +{ > > + struct intel_dp *intel_dp = intel_attached_dp(connector); > > + struct intel_digital_port *intel_dig_port = > > dp_to_dig_port(intel_dp); > > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > > + struct intel_connector *intel_connector = > > to_intel_connector(connector); > > + > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > + connector->base.id, connector->name); > > + > > + if (intel_dp->is_mst) { > > + /* MST devices are disconnected from a monitor POV */ > > + if (intel_encoder->type != INTEL_OUTPUT_EDP) > > + intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > > + return connector_status_disconnected; > > + } > > + > > + intel_dp_long_pulse(intel_dp->attached_connector); > > + > > + if (intel_connector->detect_edid) > > + return connector_status_connected; > > + else > > + return connector_status_disconnected; > > } > > > > static void
On Wednesday 13 January 2016 07:03 PM, Ander Conselvan De Oliveira wrote: > On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote: >> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: >>> intel_dp_detect() is called for not just detection but >>> during modes enumeration as well. Repeating the whole >>> sequence during each of these calls is wasteful and >>> time consuming. >>> This patch moves probing for panel, DPCD read etc done in >>> intel_dp_detect() to a new function intel_dp_long_pulse(). >>> Note that the behavior of intel_dp_detect() is changed to >>> report connected or disconnected depending on whether the >>> EDID is available or not. >>> This change will be required by further patches in the series >>> to avoid performing duplicated DPCD operations on hotplug. >>> >>> v2: Moved a hunk to next patch of the series. >>> Moved intel_dp_unset_edid to out. (Ander) >>> v3: Rephrased commit message and intel_dp_unset_dp() is called >>> within intel_dp_set_dp() to free the previous EDID. (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 | 56 +++++++++++++++++++++++++------------- >>> -- >>> - >>> 1 file changed, 35 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>> b/drivers/gpu/drm/i915/intel_dp.c >>> index 796e3d3..e3b4208 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, >>> bool sync); >>> static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp); >>> static void vlv_steal_power_sequencer(struct drm_device *dev, >>> enum pipe pipe); >>> +static void intel_dp_unset_edid(struct intel_dp *intel_dp); >>> >>> static unsigned int intel_dp_unused_lane_mask(int lane_count) >>> { >>> @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp) >>> struct intel_connector *intel_connector = intel_dp >>> ->attached_connector; >>> struct edid *edid; >>> >>> + intel_dp_unset_edid(intel_dp); >>> edid = intel_dp_get_edid(intel_dp); >>> intel_connector->detect_edid = edid; >>> >>> @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) >>> intel_dp->has_audio = false; >>> } >>> >>> -static enum drm_connector_status >>> -intel_dp_detect(struct drm_connector *connector, bool force) >>> +static void >>> +intel_dp_long_pulse(struct intel_connector *intel_connector) >>> { >>> + struct drm_connector *connector = &intel_connector->base; >>> struct intel_dp *intel_dp = intel_attached_dp(connector); >>> struct intel_digital_port *intel_dig_port = >>> dp_to_dig_port(intel_dp); >>> struct intel_encoder *intel_encoder = &intel_dig_port->base; >>> @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, bool >>> force) >>> bool ret; >>> u8 sink_irq_vector; >>> >>> - DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>> - connector->base.id, connector->name); >>> - intel_dp_unset_edid(intel_dp); >>> - >>> - if (intel_dp->is_mst) { >>> - /* MST devices are disconnected from a monitor POV */ >>> - if (intel_encoder->type != INTEL_OUTPUT_EDP) >>> - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; >>> - return connector_status_disconnected; >>> - } >>> - >>> power_domain = intel_display_port_aux_power_domain(intel_encoder); >>> intel_display_power_get(to_i915(dev), power_domain); >>> >>> @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool >>> force) >>> intel_dp_probe_oui(intel_dp); >>> >>> ret = intel_dp_probe_mst(intel_dp); >>> - if (ret) { >>> - /* if we are in MST mode then this connector >>> - won't appear connected or have anything with EDID on it >>> */ >>> - if (intel_encoder->type != INTEL_OUTPUT_EDP) >>> - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; >> This deletion is new in this version of the patch. I think we still need the >> hunk above, otherwise we might not properly update the encoder type when we >> switch from an HDMI sink connected through a level shifter to an MST sink. >> >> Ander >> Encoder type setting for MST is being done in intel_dp_detect(). So, don't find a need to add it here. >>> - status = connector_status_disconnected; >>> + if (ret) >>> goto out; > Also, there is no call to intel_dp_unset_edid() for this case, since the code > will reach the label 'out' with status being connected. So in this case the > return value of intel_dp_detect() will depend on the stale value of > intel_dp->detect_edid. > > Ander Yes.. Thats right.. Will add a call to intel_dp_unset_edid() in is_mst() check of intel_dp_detect(). > >>> - } >>> >>> /* >>> * Clearing NACK and defer counts to get their exact values >>> @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool >>> force) >>> } >>> >>> out: >>> + if (status != connector_status_connected) >>> + intel_dp_unset_edid(intel_dp); >>> intel_display_power_put(to_i915(dev), power_domain); >>> - return status; >>> + return; >>> +} >>> + >>> +static enum drm_connector_status >>> +intel_dp_detect(struct drm_connector *connector, bool force) >>> +{ >>> + struct intel_dp *intel_dp = intel_attached_dp(connector); >>> + struct intel_digital_port *intel_dig_port = >>> dp_to_dig_port(intel_dp); >>> + struct intel_encoder *intel_encoder = &intel_dig_port->base; >>> + struct intel_connector *intel_connector = >>> to_intel_connector(connector); >>> + >>> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>> + connector->base.id, connector->name); >>> + >>> + if (intel_dp->is_mst) { >>> + /* MST devices are disconnected from a monitor POV */ >>> + if (intel_encoder->type != INTEL_OUTPUT_EDP) >>> + intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; >>> + return connector_status_disconnected; >>> + } >>> + >>> + intel_dp_long_pulse(intel_dp->attached_connector); >>> + >>> + if (intel_connector->detect_edid) >>> + return connector_status_connected; >>> + else >>> + return connector_status_disconnected; >>> } >>> >>> static void
On Thu, 2016-01-14 at 19:20 +0530, Shubhangi Shrivastava wrote: > > On Wednesday 13 January 2016 07:03 PM, Ander Conselvan De Oliveira wrote: > > On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote: > > > On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: > > > > intel_dp_detect() is called for not just detection but > > > > during modes enumeration as well. Repeating the whole > > > > sequence during each of these calls is wasteful and > > > > time consuming. > > > > This patch moves probing for panel, DPCD read etc done in > > > > intel_dp_detect() to a new function intel_dp_long_pulse(). > > > > Note that the behavior of intel_dp_detect() is changed to > > > > report connected or disconnected depending on whether the > > > > EDID is available or not. > > > > This change will be required by further patches in the series > > > > to avoid performing duplicated DPCD operations on hotplug. > > > > > > > > v2: Moved a hunk to next patch of the series. > > > > Moved intel_dp_unset_edid to out. (Ander) > > > > v3: Rephrased commit message and intel_dp_unset_dp() is called > > > > within intel_dp_set_dp() to free the previous EDID. (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 | 56 +++++++++++++++++++++++++-------- > > > > ----- > > > > -- > > > > - > > > > 1 file changed, 35 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index 796e3d3..e3b4208 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp > > > > *intel_dp, > > > > bool sync); > > > > static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp); > > > > static void vlv_steal_power_sequencer(struct drm_device *dev, > > > > enum pipe pipe); > > > > +static void intel_dp_unset_edid(struct intel_dp *intel_dp); > > > > > > > > static unsigned int intel_dp_unused_lane_mask(int lane_count) > > > > { > > > > @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp) > > > > struct intel_connector *intel_connector = intel_dp > > > > ->attached_connector; > > > > struct edid *edid; > > > > > > > > + intel_dp_unset_edid(intel_dp); > > > > edid = intel_dp_get_edid(intel_dp); > > > > intel_connector->detect_edid = edid; > > > > > > > > @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > > > > intel_dp->has_audio = false; > > > > } > > > > > > > > -static enum drm_connector_status > > > > -intel_dp_detect(struct drm_connector *connector, bool force) > > > > +static void > > > > +intel_dp_long_pulse(struct intel_connector *intel_connector) > > > > { > > > > + struct drm_connector *connector = &intel_connector->base; > > > > struct intel_dp *intel_dp = intel_attached_dp(connector); > > > > struct intel_digital_port *intel_dig_port = > > > > dp_to_dig_port(intel_dp); > > > > struct intel_encoder *intel_encoder = &intel_dig_port->base; > > > > @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, > > > > bool > > > > force) > > > > bool ret; > > > > u8 sink_irq_vector; > > > > > > > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > > > - connector->base.id, connector->name); > > > > - intel_dp_unset_edid(intel_dp); > > > > - > > > > - if (intel_dp->is_mst) { > > > > - /* MST devices are disconnected from a monitor POV */ > > > > - if (intel_encoder->type != INTEL_OUTPUT_EDP) > > > > - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > > > > - return connector_status_disconnected; > > > > - } > > > > - > > > > power_domain = > > > > intel_display_port_aux_power_domain(intel_encoder); > > > > intel_display_power_get(to_i915(dev), power_domain); > > > > > > > > @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, > > > > bool > > > > force) > > > > intel_dp_probe_oui(intel_dp); > > > > > > > > ret = intel_dp_probe_mst(intel_dp); > > > > - if (ret) { > > > > - /* if we are in MST mode then this connector > > > > - won't appear connected or have anything with EDID on > > > > it > > > > */ > > > > - if (intel_encoder->type != INTEL_OUTPUT_EDP) > > > > - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > > > This deletion is new in this version of the patch. I think we still need > > > the > > > hunk above, otherwise we might not properly update the encoder type when > > > we > > > switch from an HDMI sink connected through a level shifter to an MST sink. > > > > > > Ander > > > > Encoder type setting for MST is being done in intel_dp_detect(). So, > don't find a need to add it here. Yes, but that one only covers the case where the device was already previously identified as MST. For a device identified as MST by the call to intel_dp_probe_mst() in intel_dp_long_pulse(), the encoder type override will not be done. Hopefully, Ville's patch that splits the encoder types and makes this unnecessary will land soon, but for now just leave the override there. Ander > > > > - status = connector_status_disconnected; > > > > + if (ret) > > > > goto out; > > Also, there is no call to intel_dp_unset_edid() for this case, since the > > code > > will reach the label 'out' with status being connected. So in this case the > > return value of intel_dp_detect() will depend on the stale value of > > intel_dp->detect_edid. > > > > Ander > Yes.. Thats right.. Will add a call to intel_dp_unset_edid() in is_mst() > check of intel_dp_detect(). > > > > > > - } > > > > > > > > /* > > > > * Clearing NACK and defer counts to get their exact values > > > > @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, > > > > bool > > > > force) > > > > } > > > > > > > > out: > > > > + if (status != connector_status_connected) > > > > + intel_dp_unset_edid(intel_dp); > > > > intel_display_power_put(to_i915(dev), power_domain); > > > > - return status; > > > > + return; > > > > +} > > > > + > > > > +static enum drm_connector_status > > > > +intel_dp_detect(struct drm_connector *connector, bool force) > > > > +{ > > > > + struct intel_dp *intel_dp = intel_attached_dp(connector); > > > > + struct intel_digital_port *intel_dig_port = > > > > dp_to_dig_port(intel_dp); > > > > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > > > > + struct intel_connector *intel_connector = > > > > to_intel_connector(connector); > > > > + > > > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > > > + connector->base.id, connector->name); > > > > + > > > > + if (intel_dp->is_mst) { > > > > + /* MST devices are disconnected from a monitor POV */ > > > > + if (intel_encoder->type != INTEL_OUTPUT_EDP) > > > > + intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > > > > + return connector_status_disconnected; > > > > + } > > > > + > > > > + intel_dp_long_pulse(intel_dp->attached_connector); > > > > + > > > > + if (intel_connector->detect_edid) > > > > + return connector_status_connected; > > > > + else > > > > + return connector_status_disconnected; > > > > } > > > > > > > > static void >
On Friday 15 January 2016 03:37 PM, Ander Conselvan De Oliveira wrote: > On Thu, 2016-01-14 at 19:20 +0530, Shubhangi Shrivastava wrote: >> On Wednesday 13 January 2016 07:03 PM, Ander Conselvan De Oliveira wrote: >>> On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote: >>>> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: >>>>> intel_dp_detect() is called for not just detection but >>>>> during modes enumeration as well. Repeating the whole >>>>> sequence during each of these calls is wasteful and >>>>> time consuming. >>>>> This patch moves probing for panel, DPCD read etc done in >>>>> intel_dp_detect() to a new function intel_dp_long_pulse(). >>>>> Note that the behavior of intel_dp_detect() is changed to >>>>> report connected or disconnected depending on whether the >>>>> EDID is available or not. >>>>> This change will be required by further patches in the series >>>>> to avoid performing duplicated DPCD operations on hotplug. >>>>> >>>>> v2: Moved a hunk to next patch of the series. >>>>> Moved intel_dp_unset_edid to out. (Ander) >>>>> v3: Rephrased commit message and intel_dp_unset_dp() is called >>>>> within intel_dp_set_dp() to free the previous EDID. (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 | 56 +++++++++++++++++++++++++-------- >>>>> ----- >>>>> -- >>>>> - >>>>> 1 file changed, 35 insertions(+), 21 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>>>> b/drivers/gpu/drm/i915/intel_dp.c >>>>> index 796e3d3..e3b4208 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>>> @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp >>>>> *intel_dp, >>>>> bool sync); >>>>> static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp); >>>>> static void vlv_steal_power_sequencer(struct drm_device *dev, >>>>> enum pipe pipe); >>>>> +static void intel_dp_unset_edid(struct intel_dp *intel_dp); >>>>> >>>>> static unsigned int intel_dp_unused_lane_mask(int lane_count) >>>>> { >>>>> @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp) >>>>> struct intel_connector *intel_connector = intel_dp >>>>> ->attached_connector; >>>>> struct edid *edid; >>>>> >>>>> + intel_dp_unset_edid(intel_dp); >>>>> edid = intel_dp_get_edid(intel_dp); >>>>> intel_connector->detect_edid = edid; >>>>> >>>>> @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) >>>>> intel_dp->has_audio = false; >>>>> } >>>>> >>>>> -static enum drm_connector_status >>>>> -intel_dp_detect(struct drm_connector *connector, bool force) >>>>> +static void >>>>> +intel_dp_long_pulse(struct intel_connector *intel_connector) >>>>> { >>>>> + struct drm_connector *connector = &intel_connector->base; >>>>> struct intel_dp *intel_dp = intel_attached_dp(connector); >>>>> struct intel_digital_port *intel_dig_port = >>>>> dp_to_dig_port(intel_dp); >>>>> struct intel_encoder *intel_encoder = &intel_dig_port->base; >>>>> @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, >>>>> bool >>>>> force) >>>>> bool ret; >>>>> u8 sink_irq_vector; >>>>> >>>>> - DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>>>> - connector->base.id, connector->name); >>>>> - intel_dp_unset_edid(intel_dp); >>>>> - >>>>> - if (intel_dp->is_mst) { >>>>> - /* MST devices are disconnected from a monitor POV */ >>>>> - if (intel_encoder->type != INTEL_OUTPUT_EDP) >>>>> - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; >>>>> - return connector_status_disconnected; >>>>> - } >>>>> - >>>>> power_domain = >>>>> intel_display_port_aux_power_domain(intel_encoder); >>>>> intel_display_power_get(to_i915(dev), power_domain); >>>>> >>>>> @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, >>>>> bool >>>>> force) >>>>> intel_dp_probe_oui(intel_dp); >>>>> >>>>> ret = intel_dp_probe_mst(intel_dp); >>>>> - if (ret) { >>>>> - /* if we are in MST mode then this connector >>>>> - won't appear connected or have anything with EDID on >>>>> it >>>>> */ >>>>> - if (intel_encoder->type != INTEL_OUTPUT_EDP) >>>>> - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; >>>> This deletion is new in this version of the patch. I think we still need >>>> the >>>> hunk above, otherwise we might not properly update the encoder type when >>>> we >>>> switch from an HDMI sink connected through a level shifter to an MST sink. >>>> >>>> Ander >>>> >> Encoder type setting for MST is being done in intel_dp_detect(). So, >> don't find a need to add it here. > Yes, but that one only covers the case where the device was already previously > identified as MST. For a device identified as MST by the call to > intel_dp_probe_mst() in intel_dp_long_pulse(), the encoder type override will > not be done. Hopefully, Ville's patch that splits the encoder types and makes > this unnecessary will land soon, but for now just leave the override there. > > Ander Alright.. Moved the overriding of encoder type to be done before probing.. >>>>> - status = connector_status_disconnected; >>>>> + if (ret) >>>>> goto out; >>> Also, there is no call to intel_dp_unset_edid() for this case, since the >>> code >>> will reach the label 'out' with status being connected. So in this case the >>> return value of intel_dp_detect() will depend on the stale value of >>> intel_dp->detect_edid. >>> >>> Ander >> Yes.. Thats right.. Will add a call to intel_dp_unset_edid() in is_mst() >> check of intel_dp_detect(). Added call to intel_dp_unset_edid() in is_mst() check of intel_dp_detect(). >>>>> - } >>>>> >>>>> /* >>>>> * Clearing NACK and defer counts to get their exact values >>>>> @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, >>>>> bool >>>>> force) >>>>> } >>>>> >>>>> out: >>>>> + if (status != connector_status_connected) >>>>> + intel_dp_unset_edid(intel_dp); >>>>> intel_display_power_put(to_i915(dev), power_domain); >>>>> - return status; >>>>> + return; >>>>> +} >>>>> + >>>>> +static enum drm_connector_status >>>>> +intel_dp_detect(struct drm_connector *connector, bool force) >>>>> +{ >>>>> + struct intel_dp *intel_dp = intel_attached_dp(connector); >>>>> + struct intel_digital_port *intel_dig_port = >>>>> dp_to_dig_port(intel_dp); >>>>> + struct intel_encoder *intel_encoder = &intel_dig_port->base; >>>>> + struct intel_connector *intel_connector = >>>>> to_intel_connector(connector); >>>>> + >>>>> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>>>> + connector->base.id, connector->name); >>>>> + >>>>> + if (intel_dp->is_mst) { >>>>> + /* MST devices are disconnected from a monitor POV */ >>>>> + if (intel_encoder->type != INTEL_OUTPUT_EDP) >>>>> + intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; >>>>> + return connector_status_disconnected; >>>>> + } >>>>> + >>>>> + intel_dp_long_pulse(intel_dp->attached_connector); >>>>> + >>>>> + if (intel_connector->detect_edid) >>>>> + return connector_status_connected; >>>>> + else >>>>> + return connector_status_disconnected; >>>>> } >>>>> >>>>> static void
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 796e3d3..e3b4208 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync); static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp); static void vlv_steal_power_sequencer(struct drm_device *dev, enum pipe pipe); +static void intel_dp_unset_edid(struct intel_dp *intel_dp); static unsigned int intel_dp_unused_lane_mask(int lane_count) { @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp) struct intel_connector *intel_connector = intel_dp->attached_connector; struct edid *edid; + intel_dp_unset_edid(intel_dp); edid = intel_dp_get_edid(intel_dp); intel_connector->detect_edid = edid; @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) intel_dp->has_audio = false; } -static enum drm_connector_status -intel_dp_detect(struct drm_connector *connector, bool force) +static void +intel_dp_long_pulse(struct intel_connector *intel_connector) { + struct drm_connector *connector = &intel_connector->base; struct intel_dp *intel_dp = intel_attached_dp(connector); struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); struct intel_encoder *intel_encoder = &intel_dig_port->base; @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, bool force) bool ret; u8 sink_irq_vector; - DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", - connector->base.id, connector->name); - intel_dp_unset_edid(intel_dp); - - if (intel_dp->is_mst) { - /* MST devices are disconnected from a monitor POV */ - if (intel_encoder->type != INTEL_OUTPUT_EDP) - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; - return connector_status_disconnected; - } - power_domain = intel_display_port_aux_power_domain(intel_encoder); intel_display_power_get(to_i915(dev), power_domain); @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool force) intel_dp_probe_oui(intel_dp); ret = intel_dp_probe_mst(intel_dp); - if (ret) { - /* if we are in MST mode then this connector - won't appear connected or have anything with EDID on it */ - if (intel_encoder->type != INTEL_OUTPUT_EDP) - intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; - status = connector_status_disconnected; + if (ret) goto out; - } /* * Clearing NACK and defer counts to get their exact values @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool force) } out: + if (status != connector_status_connected) + intel_dp_unset_edid(intel_dp); intel_display_power_put(to_i915(dev), power_domain); - return status; + return; +} + +static enum drm_connector_status +intel_dp_detect(struct drm_connector *connector, bool force) +{ + struct intel_dp *intel_dp = intel_attached_dp(connector); + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct intel_encoder *intel_encoder = &intel_dig_port->base; + struct intel_connector *intel_connector = to_intel_connector(connector); + + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", + connector->base.id, connector->name); + + if (intel_dp->is_mst) { + /* MST devices are disconnected from a monitor POV */ + if (intel_encoder->type != INTEL_OUTPUT_EDP) + intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; + return connector_status_disconnected; + } + + intel_dp_long_pulse(intel_dp->attached_connector); + + if (intel_connector->detect_edid) + return connector_status_connected; + else + return connector_status_disconnected; } static void
intel_dp_detect() is called for not just detection but during modes enumeration as well. Repeating the whole sequence during each of these calls is wasteful and time consuming. This patch moves probing for panel, DPCD read etc done in intel_dp_detect() to a new function intel_dp_long_pulse(). Note that the behavior of intel_dp_detect() is changed to report connected or disconnected depending on whether the EDID is available or not. This change will be required by further patches in the series to avoid performing duplicated DPCD operations on hotplug. v2: Moved a hunk to next patch of the series. Moved intel_dp_unset_edid to out. (Ander) v3: Rephrased commit message and intel_dp_unset_dp() is called within intel_dp_set_dp() to free the previous EDID. (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 | 56 +++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 21 deletions(-)