I liked the approach and agree with Daniel, so with his suggestions feel free to use: Reviewed-by: Rodrigo Vivi On Fri, Sep 4, 2015 at 7:46 AM Daniel Vetter wrote: > On Fri, Sep 04, 2015 at 06:56:12PM +0530, Sonika Jindal wrote: > > From: Shashank Sharma > > > > 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 his 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 > > > > Signed-off-by: Shashank Sharma > > Signed-off-by: Sonika Jindal > > --- > > drivers/gpu/drm/i915/intel_hdmi.c | 46 > ++++++++++++++++++++++++++++++++----- > > 1 file changed, 40 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > b/drivers/gpu/drm/i915/intel_hdmi.c > > index 5bdb386..1eda71a 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -1368,23 +1368,53 @@ intel_hdmi_set_edid(struct drm_connector > *connector) > > return connected; > > } > > > > +void intel_hdmi_probe(struct intel_encoder *intel_encoder) > > Please call it _hot_plug if it's for the _hot_plug path. > > > +{ > > + struct intel_hdmi *intel_hdmi = > > + enc_to_intel_hdmi(&intel_encoder->base); > > + struct intel_connector *intel_connector = > > + intel_hdmi->attached_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)) > > + 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); > > > > 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. > > + */ > > > > - intel_hdmi_unset_edid(connector); > > - > > - if (intel_hdmi_set_edid(connector)) { > > + 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 > > + hdmi_to_dig_port(intel_hdmi)->base.type = > INTEL_OUTPUT_HDMI; > > + DRM_DEBUG_DRIVER("hdmi status = connected\n"); > > + } else { > > status = connector_status_disconnected; > > + DRM_DEBUG_DRIVER("hdmi status = disconnected\n"); > > + } > > > > return status; > > } > > @@ -2104,11 +2134,15 @@ void intel_hdmi_init_connector(struct > intel_digital_port *intel_dig_port, > > intel_connector->unregister = intel_connector_unregister; > > > > intel_hdmi_add_properties(intel_hdmi, connector); > > + intel_encoder->hot_plug = intel_hdmi_probe; > > > > intel_connector_attach_encoder(intel_connector, intel_encoder); > > drm_connector_register(connector); > > intel_hdmi->attached_connector = intel_connector; > > > > + /* Set edid during init */ > > + intel_hdmi_probe(intel_encoder); > > Separate patch, but we really don't want this, initial probe should be > done async to avoid stalling the driver. > -Daniel > > > + > > /* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be > written > > * 0xd. Failure to do so will result in spurious interrupts being > > * generated on the port when a cable is not attached. > > -- > > 1.7.10.4 > > > > _______________________________________________ > > 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 >