On Tue, Sep 29, 2015 at 09:43:10AM +0530, Jindal, Sonika wrote:
>
>
> On 9/28/2015 7:04 PM, Daniel Vetter wrote:
> >On Mon, Sep 28, 2015 at 02:26:04PM +0530, Sonika Jindal wrote:
> >>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 and changing the patch
> >>subject. Also calling this hotplug handler from intel_hpd_init to take care
> >>of init resume scenarios.
> >>
> >>Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> >>Signed-off-by: Sonika Jindal <sonika.jindal@xxxxxxxxx>
> >>---
> >> drivers/gpu/drm/i915/intel_hdmi.c | 54 +++++++++++++++++++++++++++-------
> >> drivers/gpu/drm/i915/intel_hotplug.c | 11 +++++++
> >> 2 files changed, 54 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >>index bb33c66..9c1a308 100644
> >>--- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>+++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>@@ -1369,18 +1369,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);
> >>-
> >> while (!live_status && --retry) {
> >> live_status = intel_digital_port_connected(dev_priv,
> >> hdmi_to_dig_port(intel_hdmi));
> >>@@ -1390,15 +1388,48 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> >> 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");
> >>+}
> >>
> >>- if (intel_hdmi_set_edid(connector, live_status)) {
> >>+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.
> >>+ */
> >>+
> >>+ 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;
> >>+ DRM_DEBUG_DRIVER("hdmi status = disconnected\n");
> >>+ }
> >>
> >> return status;
> >> }
> >>@@ -2131,6 +2162,7 @@ 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_hot_plug;
> >>
> >> intel_connector_attach_encoder(intel_connector, intel_encoder);
> >> drm_connector_register(connector);
> >>diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> >>index 53c0173..36e16f6 100644
> >>--- a/drivers/gpu/drm/i915/intel_hotplug.c
> >>+++ b/drivers/gpu/drm/i915/intel_hotplug.c
> >>@@ -459,6 +459,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
> >> struct drm_device *dev = dev_priv->dev;
> >> struct drm_mode_config *mode_config = &dev->mode_config;
> >> struct drm_connector *connector;
> >>+ struct intel_encoder *encoder;
> >> int i;
> >>
> >> for_each_hpd_pin(i) {
> >>@@ -482,6 +483,16 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
> >> if (dev_priv->display.hpd_irq_setup)
> >> dev_priv->display.hpd_irq_setup(dev);
> >> spin_unlock_irq(&dev_priv->irq_lock);
> >>+
> >>+ /*
> >>+ * Connected boot / resume scenarios can't generate new hot plug.
> >>+ * So, probe it manually.
> >>+ */
> >>+ list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> >>+ base.head) {
> >>+ if (encoder->hot_plug)
> >>+ encoder->hot_plug(encoder);
> >>+ }
> >
> >Since this doesn't just change hdmi but every encoder I think we should
> >pull it out into a separate prep patch. Especially interactions with dp
> >mst handling look tricky, have you tested that nothing blows up with that?
> >The split-out patch should definitely explain in detail how this interacts
> >with dp hpd handling.
> >-Daniel
> >
> Right now we don't set hot_plug for DP/edp encoder.
> It was used only for sdvo and with this patch we add for hdmi.
> Also, in sdvo_init right after assigning the hot_plug function pointer, we
> call its hot_plug function for initialization.
> Maybe with this patch, we can remove that part also from sdvo_init.
Hm right, I forgot that we have too many hotplug callbacks in our driver
;-)
> I can split this patch but without this change, it will result in regression
> with just one patch. So I thought it would be better to keep them together
> in one patch.
If you split it out and make it patch 1 and then the hdmi changes patch 2
nothing breaks in between.
-Daniel
--
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
