On Tue, 2015-11-17 at 11:47 +0530, Shubhangi Shrivastava wrote:
>
> On Monday 16 November 2015 07:10 PM, Ander Conselvan De Oliveira wrote:
> > On Fri, 2015-11-06 at 17:58 +0530, Shubhangi Shrivastava wrote:
> > > This patch moves probing for panel, DPCD read etc to another
> > > function intel_dp_long_pulse, while intel_dp_detect returns
> > > the status as connected or disconnected depending on
> > > whether the edid is available or not.
> > Why is the change in connector status necessary?
> Since intel_dp_detect is also called in mode enumeration, so all detect
> related operations are moved to intel_dp_long_pulse to avoid any such
> operations to be performed during mode enumeration. intel_dp_long_pulse
> will perform all the panel probing, DPCD read operations etc. which will
> end with EDID being read. In intel_dp_detect, it is checked if EDID has
> been read or not. If EDID has been read then the status is returned as
> connected to allow further operations otherwise the status is returned
> as disconnected.
So if I understood correctly, the EDID change is only a about doing a better
split of the code. My concern is that drm_get_edid() might fail. Before this
patch that wouldn't affect the connector status, but now a failure to read EDID
implies that the connector is disconnected. I'm not sure in which scenario this
would happen, so I wanted to know if this change in behavior is intentional.
Ander
>
> >
> > > This change will be required by further patches in the series
> > > to avoid performing multiple DPCD operations and removing
> > > their duplication.
> > >
> > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@xxxxxxxxx>
> > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++++-----
> > > -----
> > > -
> > > 1 file changed, 49 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 1cb1f3f..a0fe827 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4785,9 +4785,10 @@ intel_dp_power_put(struct intel_dp *dp,
> > > intel_display_power_put(to_i915(encoder->base.dev),
> > > power_domain);
> > > }
> > >
> > > -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;
> > > @@ -4797,17 +4798,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_dp_power_get(intel_dp);
> > >
> > > /* Can't disconnect eDP, but you can close the lid... */
> > > @@ -4817,19 +4807,35 @@ intel_dp_detect(struct drm_connector *connector,
> > > bool
> > > force)
> > > status = ironlake_dp_detect(intel_dp);
> > > else
> > > status = g4x_dp_detect(intel_dp);
> > > - if (status != connector_status_connected)
> > > + if (status != connector_status_connected) {
> > > + intel_dp_unset_edid(intel_dp);
> > > goto out;
> > > + }
> > >
> > > 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 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;
> > > goto out;
> > Here the function will exit without a call to intel_dp_unset_edid(), which
> > is
> > different from the behavior prior to this patch.
> Moved call to intel_dp_unset_edid() to out, as per next comment.
> >
> > > + } 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;
> > > }
> > The hunk above should be in a separate patch. It also doesn't call
> > intel_dp_unset_edid(). Might be easier to just add
> >
> > out:
> > if (status == connector_status_disconnected)
> > intel_dp_unset_edid(intel_dp);
> >
> > to the end of the function.
> >
> > Ander
> Done. Moved the hunk to next patch and call to intel_dp_unset_edid to out.
> >
> > > intel_dp_set_edid(intel_dp);
> > > @@ -4854,7 +4860,33 @@ intel_dp_detect(struct drm_connector *connector,
> > > bool
> > > force)
> > >
> > > out:
> > > intel_dp_power_put(intel_dp, 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
