By "we" I meant me :) Also, since it isn't possible to check all the platforms for live status, it is safer to put a gen check to platforms which we are sure. Anyways, lets see if increasing the delay helps there, then you can take a call. Regards, Sonika -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel Vetter Sent: Thursday, December 10, 2015 2:24 PM To: Jindal, Sonika <sonika.jindal@xxxxxxxxx> Cc: Daniel Vetter <daniel@xxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx Subject: Re: [PATCH] drm/i915: Add hot_plug hook for hdmi encoder On Thu, Dec 10, 2015 at 08:35:30AM +0000, Jindal, Sonika wrote: > Sure, I will start a new thread for this. > > Regarding the regression, since it is a sandybridge system, I am not > sure if I can debug it anyhow because of the unavailability of the > system. For the live status check, we earlier suggested to use > platforms gen8 and above. > But for wider testing, we removed the check. > If required we can add a check there. It's your patch, you need to own this. And until we've confirmed that it's a source issue I won't add a genAnything check but instead just revert the patch. I hope expectations from my side here are clear. Thanks, Daniel > > Regards, > Sonika > > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of > Daniel Vetter > Sent: Thursday, December 10, 2015 2:00 PM > To: Jindal, Sonika <sonika.jindal@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH] drm/i915: Add hot_plug hook for hdmi > encoder > > On Thu, Dec 10, 2015 at 10:15:41AM +0530, Sonika Jindal wrote: > > From: Shashank Sharma <shashank.sharma@xxxxxxxxx> > > > > This patch adds a separate probe function for HDMI EDID read over > > DDC channel. This function has been registered as a .hot_plug > > handler for HDMI encoder. > > > > The current implementation of hdmi_detect() function re-sets the > > cached HDMI edid (in connector->detect_edid) in every detect > > call.This function gets called many times, sometimes directly from > > userspace probes, forcing drivers to read EDID every detect function > > call.This causes several problems like: > > > > 1. Race conditions in multiple hot_plug / unplug cases, between > > interrupts bottom halves and userspace detections. > > 2. Many Un-necessary EDID reads for single hotplug/unplug 3. HDMI > > complaince failures which expects only one EDID read per hotplug > > > > This function will be serving the purpose of really reading the EDID > > by really probing the DDC channel, and updating the cached EDID. > > > > The plan is to: > > 1. i915 IRQ handler bottom half function already calls > > intel_encoder->hotplug() function. Adding This probe function which > > will read the EDID only in case of a hotplug / unplug. > > 2. During init_connector this probe will be called to read the edid 3. > > Reuse the cached EDID in hdmi_detect() function. > > > > The "< gen7" check is there because this was tested only for >=gen7 > > platforms. For older platforms the hotplug/reading edid path remains same. > > > > v2: Calling set_edid instead of hdmi_probe during init. > > Also, for platforms having DDI, intel_encoder for DP and HDMI is > > same (taken from intel_dig_port), so for DP also, hot_plug function > > gets called which is not intended here. So, check for HDMI in > > intel_hdmi_probe Rely on HPD for updating edid only for platforms gen > 8 and also for VLV. > > > > v3: Dropping the gen < 8 || !VLV check. Now all platforms should > > rely on hotplug or init for updating the edid.(Daniel) Also, calling > > hdmi_probe in init instead of set_edid > > > > v4: Renaming intel_hdmi_probe to intel_hdmi_hot_plug. > > Also calling this hotplug handler from intel_hpd_init to take care > > of init resume scenarios. > > > > v5: Moved the call to encoder hotplug during init to separate > > patch(Daniel) > > v6: Rebased and maintaining authorship. > > > > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> > > Signed-off-by: Sonika Jindal <sonika.jindal@xxxxxxxxx> > > Iirc we had to revert an earlier version of this. Commit message should cite that revert and explain why this version is better. > > Also after a few days please start a new thread when resubmitting a patch series. The idea behind in-reply-to is to keep the discussion together while it's still ongoing. That's not the case when the discussion has died down since weeks. > > Also, your patch to take hpd sense into account for hdmi detection has causes a regression. Your on the cc. Handling that regression needs to be absolute top priority (otherwise I need to revert that patch too). > > Thanks, Daniel > > > --- > > drivers/gpu/drm/i915/intel_hdmi.c | 58 > > ++++++++++++++++++++++++++++++--------- > > 1 file changed, 45 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > > b/drivers/gpu/drm/i915/intel_hdmi.c > > index bdd462e..16dd2a7 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -1381,18 +1381,16 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force) > > return connected; > > } > > > > -static enum drm_connector_status > > -intel_hdmi_detect(struct drm_connector *connector, bool force) > > +void intel_hdmi_hot_plug(struct intel_encoder *intel_encoder) > > { > > - enum drm_connector_status status; > > - struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > > - struct drm_i915_private *dev_priv = to_i915(connector->dev); > > + struct intel_hdmi *intel_hdmi = > > + 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; > > > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > - connector->base.id, connector->name); > > - > > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > > > > while (!live_status && --retry) { > > @@ -1400,21 +1398,53 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > > hdmi_to_dig_port(intel_hdmi)); > > mdelay(10); > > } > > + intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); > > > > if (!live_status) > > DRM_DEBUG_KMS("Live status not up!"); > > > > - intel_hdmi_unset_edid(connector); > > + /* > > + * 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, live_status)) > > + DRM_DEBUG_DRIVER("DDC probe: got EDID\n"); > > + else > > + DRM_DEBUG_DRIVER("DDC probe: no EDID\n"); } > > + > > +static enum drm_connector_status > > +intel_hdmi_detect(struct drm_connector *connector, bool force) { > > + enum drm_connector_status status; > > + struct intel_connector *intel_connector = > > + to_intel_connector(connector); > > > > - if (intel_hdmi_set_edid(connector, live_status)) { > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > + connector->base.id, connector->name); > > + > > + /* > > + * There are many userspace calls which probe EDID from > > + * detect path. In case of multiple hotplug/unplug, these > > + * can cause race conditions while probing EDID. Also its > > + * waste of CPU cycles to read the EDID again and again > > + * unless there is a real hotplug. > > + * So, rely on hotplugs and init to read edid. > > + * Check connector status based on availability of cached EDID. > > + */ > > + > > + if (intel_connector->detect_edid) { > > struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > > > > hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI; > > status = connector_status_connected; > > - } else > > + DRM_DEBUG_DRIVER("hdmi status = connected\n"); > > + } else { > > status = connector_status_disconnected; > > - > > - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); > > + DRM_DEBUG_DRIVER("hdmi status = disconnected\n"); > > + } > > > > return status; > > } > > @@ -2131,6 +2161,8 @@ void intel_hdmi_init_connector(struct > > intel_digital_port *intel_dig_port, > > > > intel_hdmi_add_properties(intel_hdmi, connector); > > > > + intel_encoder->hot_plug = intel_hdmi_hot_plug; > > + > > intel_connector_attach_encoder(intel_connector, intel_encoder); > > drm_connector_register(connector); > > intel_hdmi->attached_connector = intel_connector; > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
