Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx