| Message ID | 1441970796-5850-1-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > --- > 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); > > /* 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 >
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
On 9/14/2015 2:12 PM, Daniel Vetter wrote: > 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 > Hmm, since rebasing this will require these changes (check for live status) to go in detect and then later when we reach a conclusion on hot_plug hook, we would need to move it to that function. I think lets wait for the conclusion on the placement of that function. What do you suggest? Regards, Sonika >>> --- >>> 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 > >
On Mon, Sep 14, 2015 at 02:44:01PM +0530, Jindal, Sonika wrote: > > > On 9/14/2015 2:12 PM, Daniel Vetter wrote: > >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 > > > Hmm, since rebasing this will require these changes (check for live status) > to go in detect and then later when we reach a conclusion on > hot_plug hook, we would need to move it to that function. > I think lets wait for the conclusion on the placement of that function. > What do you suggest? I think given how tricky this patch here is with testing it's preferably to get it int as soon as possible. Yes that means shuffling things around twice. But since the structure of how we'll handle hpd isn't clear yet at all there's a really high chance that you need to rework this patch anyway. So I don't expect doing this rebase right now will be more work. And it will give us better testing coverage, which is always good to have. Thanks, Daniel
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); /* 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; }
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> --- 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(-)