Re: [PATCH] drm/i915: Check live status before reading edid

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Sep 11, 2015 at 05:56:47PM +0000, Rodrigo Vivi wrote:
> Thanks
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> 
> On Fri, Sep 11, 2015 at 4:38 AM Sonika Jindal <sonika.jindal@intel.com>
> wrote:
> 
> > The Bspec is very clear that Live status must be checked about before
> > trying to read EDID over DDC channel. This patch makes sure that HDMI
> > EDID is read only when live status is up.
> >
> > The live status doesn't seem to perform very consistent across various
> > platforms when tested with different monitors. The reason behind that is
> > some monitors are late to provide right voltage to set live_status up.
> > So, after getting the interrupt, for a small duration, live status reg
> > fluctuates, and then settles down showing the correct staus.
> >
> > This is explained here in, in a rough way:
> > HPD line  ________________
> >                          |\ T1 = Monitor Hotplug causing IRQ
> >                          | \______________________________________
> >                          | |
> >                          | |
> >                          | |   T2 = Live status is stable
> >                          | |  _____________________________________
> >                          | | /|
> > Live status _____________|_|/ |
> >                          | |  |
> >                          | |  |
> >                          | |  |
> >                         T0 T1  T2
> >
> > (Between T1 and T2 Live status fluctuates or can be even low, depending on
> >  the monitor)
> >
> > After several experiments, we have concluded that a max delay
> > of 30ms is enough to allow the live status to settle down with
> > most of the monitors. This total delay of 30ms has been split into
> > a resolution of 3 retries of 10ms each, for the better cases.
> >
> > This delay is kept at 30ms, keeping in consideration that, HDCP compliance
> > expect the HPD handler to respond a plug out in 100ms, by disabling port.
> >
> > v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
> > to check digital port status. Adding a separate function to get bxt live
> > status (Daniel)
> > v3: Using intel_encoder->hpd_pin to check the live status (Siva)
> > Moving the live status read to intel_hdmi_probe and passing parameter
> > to read/not to read the edid. (me)
> > v4:
> > * Added live status check for all platforms using
> > intel_digital_port_connected.
> > * Rebased on top of Jani's DP cleanup series
> > * Some monitors take time in setting the live status. So retry for few
> > times if this is a connect HPD
> > v5: Removed extra "drm/i915" from commit message. Adding Shashank's sob
> >     which was missed.
> > v6: Drop the (!detect_edid && !live_status check) check because for DDI
> > ports which are enumerated as hdmi as well as DP, we don't have a
> > mechanism to differentiate between DP and hdmi inside the encoder's
> > hot_plug. This leads to call to the hdmi's hot_plug hook for DP as well
> > as hdmi which leads to issues during unplug because of the above check.
> > v7: Make intel_digital_port_connected global in this patch, some
> > reformatting of while loop, adding a print when live status is not
> > up. (Rodrigo)
> >
> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>

Since this is tricky stuff and other patches in this series are blocked
until we have a clearer picture can you please rebase this to be the first
patch on top of -nightly so that I can pull it in directly?

I tried to do that but proved a bit too messy.
-Daniel

> > ---
> >  drivers/gpu/drm/i915/intel_dp.c   |    2 +-
> >  drivers/gpu/drm/i915/intel_drv.h  |    2 ++
> >  drivers/gpu/drm/i915/intel_hdmi.c |   26 +++++++++++++++++++-------
> >  3 files changed, 22 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index bf17030..fedf6d1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4695,7 +4695,7 @@ static bool bxt_digital_port_connected(struct
> > drm_i915_private *dev_priv,
> >   *
> >   * Return %true if @port is connected, %false otherwise.
> >   */
> > -static bool intel_digital_port_connected(struct drm_i915_private
> > *dev_priv,
> > +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> >                                          struct intel_digital_port *port)
> >  {
> >         if (HAS_PCH_IBX(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index b6c2c20..ac6d748 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1210,6 +1210,8 @@ void intel_edp_drrs_disable(struct intel_dp
> > *intel_dp);
> >  void intel_edp_drrs_invalidate(struct drm_device *dev,
> >                 unsigned frontbuffer_bits);
> >  void intel_edp_drrs_flush(struct drm_device *dev, unsigned
> > frontbuffer_bits);
> > +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> > +                                        struct intel_digital_port *port);
>g>
> >  /* intel_dp_mst.c */
> >  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port,
> > int conn_id);
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 1eda71a..d366ca5 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector
> > *connector)
> >  }
> >
> >  static bool
> > -intel_hdmi_set_edid(struct drm_connector *connector)
> > +intel_hdmi_set_edid(struct drm_connector *connector, bool force)
> >  {
> >         struct drm_i915_private *dev_priv = to_i915(connector->dev);
> >         struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> >         struct intel_encoder *intel_encoder =
> >                 &hdmi_to_dig_port(intel_hdmi)->base;
> >         enum intel_display_power_domain power_domain;
> > -       struct edid *edid;
> > +       struct edid *edid = NULL;
> >         bool connected = false;
> >
> >         power_domain = intel_display_port_power_domain(intel_encoder);
> >         intel_display_power_get(dev_priv, power_domain);
> >
> > -       edid = drm_get_edid(connector,
> > -                           intel_gmbus_get_adapter(dev_priv,
> > -                                                   intel_hdmi->ddc_bus));
> > +       if (force)
> > +               edid = drm_get_edid(connector,
> > +                                   intel_gmbus_get_adapter(dev_priv,
> > +                                   intel_hdmi->ddc_bus));
> >
> >         intel_display_power_put(dev_priv, power_domain);
> >
> > @@ -1374,14 +1375,25 @@ void intel_hdmi_probe(struct intel_encoder
> > *intel_encoder)
> >                         enc_to_intel_hdmi(&intel_encoder->base);
> >         struct intel_connector *intel_connector =
> >                                 intel_hdmi->attached_connector;
> > +       struct drm_i915_private *dev_priv =
> > to_i915(intel_encoder->base.dev);
> > +       bool live_status = false;
> > +       unsigned int retry = 3;
> > +
> > +       while (!live_status && --retry) {
> > +               live_status = intel_digital_port_connected(dev_priv,
> > +                               hdmi_to_dig_port(intel_hdmi));
> > +               mdelay(10);
> > +       }
> >
> > +       if (!live_status)
> > +               DRM_DEBUG_KMS("Live status not up!");
> >         /*
> >          * We are here, means there is a hotplug or a force
> >          * detection. Clear the cached EDID and probe the
> >          * DDC bus to check the current status of HDMI.
> >          */
> >         intel_hdmi_unset_edid(&intel_connector->base);
> > -       if (intel_hdmi_set_edid(&intel_connector->base))
> > +       if (intel_hdmi_set_edid(&intel_connector->base, live_status))
> >                 DRM_DEBUG_DRIVER("DDC probe: got EDID\n");
> >         else
> >                 DRM_DEBUG_DRIVER("DDC probe: no EDID\n");
> > @@ -1432,7 +1444,7 @@ intel_hdmi_force(struct drm_connector *connector)
> >         if (connector->status != connector_status_connected)
> >                 return;
> >
> > -       intel_hdmi_set_edid(connector);
> > +       intel_hdmi_set_edid(connector, true);
> >         hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
> >  }
> >
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >

> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx