[PATCH 0/2] Optimization on intel HDMI detect and get_modes

[PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Ramalingam C]

On slow HDMI hotplug out, because of the physical interface design,
DDC remains active for short duration even when HPD live status is
indicating the disconnect state. Because of this on VLV and HSW,
slow hotplug out events are not captured.

Hence this change uses the HPD pins live status to identify the
HDMI connector status.

Multiple forced detect and read modes calls come from
user space for different connectors, which can be handled
with connector status and previous detect event's cached EDID.
With this approach for hot pluggable displays, EDID retrieval
is required only when there is a real hot-plug events.

This change also includes:
1. A logic to optimize those multiple calls, by re-using
   cached data from previous detect and get_mode calls.
2. Store HDMI EDID, and re-use it in read modes.
3. Read live status reg to suppress spurious interrupts

These two changes will optimize the access to the DDC and also the
CPU cycles burned by intel_hdmi_detect and intel_hdmi_get_modes.

Ramalingam C (1):
  drm/i915: HDMI detection based on HPD pin live status

Shashank Sharma (1):
  drm/i915: Optimize EDID retrival on detect and get_modes

 drivers/gpu/drm/i915/intel_drv.h  |   12 +++
 drivers/gpu/drm/i915/intel_hdmi.c |  149 ++++++++++++++++++++++++++++++++-----
 2 files changed, 144 insertions(+), 17 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] drm/i915: HDMI detection based on HPD pin live status

[Ramalingam C]

This change uses the HPD pins live status bit from
South Display Engine(SDE) to identify the HDMI hotplug state.

On Soft HPD events (on automated test cases) only HPD pin will be
toggled to notify the HDMI state change. But physical DDC will
be alive. Similarly on slow HDMI hotplug out, because of the physical
interface design, DDC remains active for short duration even when
HPD live status is indicating the disconnect state. Because of this
on VLV and HSW, slow hotplug out events and soft HPDs are not captured.

Hence this patch uses the HPD pins live status to identify the
HDMI connector status and allows EDID retrival only when live status
is up.

Change-Id: I958b57fa139e52b45c8b349c861cb8eab7b67ae5
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |   10 +++++
 drivers/gpu/drm/i915/intel_hdmi.c |   87 ++++++++++++++++++++++++++++++++-----
 2 files changed, 85 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 46aea6c..0f7d94b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -444,6 +444,16 @@ struct cxsr_latency {
 #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
 #define to_intel_plane(x) container_of(x, struct intel_plane, base)
 
+/* DisplayPort/HDMI Hotplug line status bit mask */
+#define VLV_HDMIB_HOTPLUG_LIVE_STATUS   (1 << 29)
+#define VLV_HDMIC_HOTPLUG_LIVE_STATUS   (1 << 28)
+#define VLV_HDMID_HOTPLUG_LIVE_STATUS   (1 << 27)
+
+/* DisplayPort/HDMI/DVI Hotplug line status bit mask */
+#define CORE_HDMIB_HOTPLUG_LIVE_STATUS  (1 << 21)
+#define CORE_HDMIC_HOTPLUG_LIVE_STATUS  (1 << 22)
+#define CORE_HDMID_HOTPLUG_LIVE_STATUS  (1 << 23)
+
 struct intel_hdmi {
 	u32 hdmi_reg;
 	int ddc_bus;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 6db0d9d..faeae3a 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -924,6 +924,61 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	return true;
 }
 
+static int get_hdmi_hotplug_live_status(struct drm_device *dev,
+					struct intel_hdmi *intel_hdmi)
+{
+	uint32_t bit, reg;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_digital_port *intel_dig_port =
+				hdmi_to_dig_port(intel_hdmi);
+
+	DRM_DEBUG_KMS("Reading Live status");
+
+	/* Live status is available from Gen 6 onwards */
+	if (INTEL_INFO(dev)->gen < 6)
+		return connector_status_connected;
+
+	if (IS_VALLEYVIEW(dev)) {
+		switch (intel_dig_port->port) {
+		case PORT_B:
+			bit = VLV_HDMIB_HOTPLUG_LIVE_STATUS;
+			break;
+		case PORT_C:
+			bit = VLV_HDMIC_HOTPLUG_LIVE_STATUS;
+			break;
+		case PORT_D:
+			bit = VLV_HDMID_HOTPLUG_LIVE_STATUS;
+			break;
+		default:
+			DRM_ERROR("Unrecognized port is encountered\n");
+			return connector_status_unknown;
+		}
+		reg = I915_READ(PORT_HOTPLUG_STAT);
+
+	} else {
+		switch (intel_dig_port->port) {
+		case PORT_B:
+			bit = CORE_HDMIB_HOTPLUG_LIVE_STATUS;
+			break;
+		case PORT_C:
+			bit = CORE_HDMIC_HOTPLUG_LIVE_STATUS;
+			break;
+		case PORT_D:
+			bit = CORE_HDMID_HOTPLUG_LIVE_STATUS;
+			break;
+		default:
+			DRM_ERROR("Unrecognized port is encountered\n");
+			return connector_status_unknown;
+		}
+
+		reg = I915_READ(SDEISR);
+	}
+
+	/* Return connector status */
+	return ((reg & bit) ?
+		connector_status_connected : connector_status_disconnected);
+}
+
 static enum drm_connector_status
 intel_hdmi_detect(struct drm_connector *connector, bool force)
 {
@@ -939,24 +994,32 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, drm_get_connector_name(connector));
 
+	status = get_hdmi_hotplug_live_status(dev, intel_hdmi);
+
 	intel_hdmi->has_hdmi_sink = false;
 	intel_hdmi->has_audio = false;
 	intel_hdmi->rgb_quant_range_selectable = false;
-	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
 
-	if (edid) {
-		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
-			status = connector_status_connected;
-			if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
-				intel_hdmi->has_hdmi_sink =
+	if (status == connector_status_connected) {
+		edid = drm_get_edid(connector,
+				intel_gmbus_get_adapter(dev_priv,
+						intel_hdmi->ddc_bus));
+		if (edid) {
+			if (edid->input & DRM_EDID_INPUT_DIGITAL) {
+				status = connector_status_connected;
+				if (intel_hdmi->force_audio !=
+							HDMI_AUDIO_OFF_DVI)
+					intel_hdmi->has_hdmi_sink =
 						drm_detect_hdmi_monitor(edid);
-			intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
-			intel_hdmi->rgb_quant_range_selectable =
-				drm_rgb_quant_range_selectable(edid);
+				intel_hdmi->has_audio =
+						drm_detect_monitor_audio(edid);
+				intel_hdmi->rgb_quant_range_selectable =
+					drm_rgb_quant_range_selectable(edid);
+			}
+			kfree(edid);
+		} else {
+			status = connector_status_disconnected;
 		}
-		kfree(edid);
 	}
 
 	if (status == connector_status_connected) {
-- 
1.7.9.5

[PATCH 2/2] drm/i915: Optimize EDID retrival on detect and get_modes

[Ramalingam C]

From: Shashank Sharma <shashank.sharma@intel.com>

Multiple forced detect and read modes calls come from
user space for different connectors, which can be handled
with connector status and previous detect event's cached EDID.
With this approach for hot pluggable displays, EDID retrieval
is required only when there is a real hot-plug events.

This approach optimizes access to the DDC interface and also the
CPU cycles burned by intel_hdmi_detect and intel_hdmi_get_modes.

This patch contains:
1. A logic to optimize those multiple calls, by re-using
   cached data from previous detect and get_mode calls.
2. Store HDMI EDID, and re-use it in read modes.
3. Read live status reg to suppress spurious interrupts

Change-Id: Ia46cbe346dcc18ef11381eabcb157c5bcfd51322
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |    2 ++
 drivers/gpu/drm/i915/intel_hdmi.c |   66 +++++++++++++++++++++++++++++++++----
 2 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0f7d94b..4f7f81f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -463,6 +463,8 @@ struct intel_hdmi {
 	bool has_audio;
 	enum hdmi_force_audio force_audio;
 	bool rgb_quant_range_selectable;
+	struct edid *edid;
+	uint32_t edid_mode_count;
 	void (*write_infoframe)(struct drm_encoder *encoder,
 				enum hdmi_infoframe_type type,
 				const void *frame, ssize_t len);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index faeae3a..2d008b7 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -994,7 +994,14 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, drm_get_connector_name(connector));
 
+	/* If its forced detect call, dont read EDID again */
+	if (force && intel_hdmi->edid)
+		return connector->status;
+
 	status = get_hdmi_hotplug_live_status(dev, intel_hdmi);
+	/* Suppress spurious IRQ, if current status is same as live status*/
+	if (connector->status == status)
+		return connector->status;
 
 	intel_hdmi->has_hdmi_sink = false;
 	intel_hdmi->has_audio = false;
@@ -1015,11 +1022,22 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 						drm_detect_monitor_audio(edid);
 				intel_hdmi->rgb_quant_range_selectable =
 					drm_rgb_quant_range_selectable(edid);
+
+				/* Free previously saved EDID and save new one
+				   for read modes. */
+				kfree(intel_hdmi->edid);
+				intel_hdmi->edid = edid;
+			} else {
+				kfree(edid);
+				DRM_ERROR("EDID is not in digital form ?\n");
 			}
-			kfree(edid);
 		} else {
-			status = connector_status_disconnected;
+			DRM_DEBUG_KMS("EDID read failed\n");
 		}
+
+		if (intel_hdmi->edid == NULL)
+			status = connector_status_disconnected;
+
 	}
 
 	if (status == connector_status_connected) {
@@ -1027,6 +1045,9 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 			intel_hdmi->has_audio =
 				(intel_hdmi->force_audio == HDMI_AUDIO_ON);
 		intel_encoder->type = INTEL_OUTPUT_HDMI;
+	} else {
+		kfree(intel_hdmi->edid);
+		intel_hdmi->edid = NULL;
 	}
 
 	return status;
@@ -1036,14 +1057,44 @@ static int intel_hdmi_get_modes(struct drm_connector *connector)
 {
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 	struct drm_i915_private *dev_priv = connector->dev->dev_private;
+	struct edid *edid = intel_hdmi->edid;
+	struct drm_display_mode *mode = NULL;
+	int count = 0;
 
-	/* We should parse the EDID data and find out if it's an HDMI sink so
-	 * we can send audio to it.
-	 */
+	/* No need to read modes if panel is not connected */
+	if (connector->status != connector_status_connected)
+		return 0;
 
-	return intel_ddc_get_modes(connector,
+	/* Need not read modes again if previously read modes are
+	   available and display is consistent */
+	if (intel_hdmi->edid) {
+		list_for_each_entry(mode, &connector->modes, head) {
+			if (mode) {
+				/* Setting the MODE_OK for all sanitized modes*/
+				mode->status = MODE_OK;
+				count++;
+			}
+		}
+		/* If modes are availlable, no need to read again */
+		if (count)
+			goto out;
+	}
+
+	/* EDID was saved in detect, re-use that if available, avoid
+	   reading EDID everytime */
+	if (edid) {
+		drm_mode_connector_update_edid_property(connector, edid);
+		count = drm_add_edid_modes(connector, edid);
+		drm_edid_to_eld(connector, edid);
+	} else {
+		count = intel_ddc_get_modes(connector,
 				   intel_gmbus_get_adapter(dev_priv,
-							   intel_hdmi->ddc_bus));
+						   intel_hdmi->ddc_bus));
+	}
+
+out:
+	intel_hdmi->edid_mode_count = count;
+	return count;
 }
 
 static bool
@@ -1381,6 +1432,7 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 
 	intel_dig_port->port = port;
 	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
+	intel_dig_port->hdmi.edid = NULL;
 	intel_dig_port->dp.output_reg = 0;
 
 	intel_hdmi_init_connector(intel_dig_port, intel_connector);
-- 
1.7.9.5

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Daniel Vetter]

On Mon, Jan 13, 2014 at 12:21:52PM +0530, Ramalingam C wrote:
> On slow HDMI hotplug out, because of the physical interface design,
> DDC remains active for short duration even when HPD live status is
> indicating the disconnect state. Because of this on VLV and HSW,
> slow hotplug out events are not captured.
> 
> Hence this change uses the HPD pins live status to identify the
> HDMI connector status.
> 
> Multiple forced detect and read modes calls come from
> user space for different connectors, which can be handled
> with connector status and previous detect event's cached EDID.
> With this approach for hot pluggable displays, EDID retrieval
> is required only when there is a real hot-plug events.
> 
> This change also includes:
> 1. A logic to optimize those multiple calls, by re-using
>    cached data from previous detect and get_mode calls.
> 2. Store HDMI EDID, and re-use it in read modes.
> 3. Read live status reg to suppress spurious interrupts
> 
> These two changes will optimize the access to the DDC and also the
> CPU cycles burned by intel_hdmi_detect and intel_hdmi_get_modes.
> 
> Ramalingam C (1):
>   drm/i915: HDMI detection based on HPD pin live status
> 
> Shashank Sharma (1):
>   drm/i915: Optimize EDID retrival on detect and get_modes
> 
>  drivers/gpu/drm/i915/intel_drv.h  |   12 +++
>  drivers/gpu/drm/i915/intel_hdmi.c |  149 ++++++++++++++++++++++++++++++++-----
>  2 files changed, 144 insertions(+), 17 deletions(-)

Nak.

We've had code to use the hpd live status register on all platforms, but
had to take it out again on all platforms due to broken hardware: The live
status simply doesn't work sometimes, resulting in angry users reporting
black screens. So as-is patch 1 can't go into upstream. I know that it'd
be really useful if we could rely on the hpd live status but, but thus far
I couldn't come up with a good idea to make it work. Git history should
have all the details.

Also note that machines with noisy interrupt lines (see the interrupt
storm handling code in i915_irq.c) complicate this further.

This means that patch 2 is also a no-go since we really can't rely upon
the hpd stuff for hdmi detection. But we can save EDID caching if we
automatically invalidate the cached edid after 1-2 seconds or the next hpd
interrupt (whichever is first). 1-2 seconds should be long enough for
userspace to read the EDID a few times and change the output
configuration, but not long enough for users to get pissed. Note that the
caching interval should be shorter than the polling interval, which we use
as a fallback if the hpd lines are noise and atm is at 10 seconds.

Also this EDID caching and invalidation code should imo be a generic
helper with data structures (for the invalidation work and cached edid) in
struct intel_output. That way we can also wire it up for e.g. VGA or any
other output that'll benefit from EDID caching.

Aside: On DP the hpd pins seem to be reliable thus far, but I'm not sure
whether that's just lack of test coverage (since DP screens are less
common) or because it actually works ...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Sharma, Shashank]

Hello Daniel

Thanks a lot for your time, for reviewing the changes, and giving us some pointers. 
Both me and Ramalingam are designing this together, and we discussed about these changes and your suggestions. 
There are few things we would like to discuss about. Please correct us if some of our understanding is not proper. 

Those two patches provide two solution. 
1. Support for soft HPD, and slow removal of HDMI (when the DDC channel can still get the EDID).    
2. Try to reduce the EDID reads over DDC channel for get connector and fill mode calls, by caching EDID, and using it until next HPD comes.

Patch 2: Reduce the EDID read over DDC channel
We are caching the EDID at every HPD up, on HDMI detect calls, and we are freeing it on subsequent HDMI disconnect calls.

The design philosophy here is, to maintain a state machine of HDMI connector status, and differentiate between IOCTL detect calls and HPD detect calls. 
If there is a detect() or get_modes() call due to any of the IOCTL, which makes sure that input variable force=1, we just use the cached EDID, to serve this calls. 
But if the detect call is coming from HPD work function, due to a HPD plug-out, we remove/invalidate the old cached EDID, and cache the new EDID, on subsequent HDMI plug-in. 
>From here, the same state machine follows. 

Can you please let us know, why do you think that we should invalidate the cached EDID after 1-2 seconds ?

Note: In this same patch, there is additional optimization, which you pointed out, where we check if the connector->status is same as live status. 
This can be removed independently, as you suggested.   

About patch 1: 
We have done some local experiments and we came to know that for VLV and HSW boards, we can rely on the live status, if we give it some time to settle (~300ms). 
Probably, we need to modify this patch, as you suggested, until it becomes handy to be used reliably. We are on it, and will send another patch soon.
 
But if somehow we are able to get some consistent results from live status, do you think it would be worth accepting this change, so that it can handle soft HPDs and automation testing. 
Because I believe we will face this problem whenever we are trying to test something from automation, where the physical device is not removed, and DDC channel is up always. 

Regards
Shashank 

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Monday, January 13, 2014 12:59 PM
To: C, Ramalingam
Cc: intel-gfx@lists.freedesktop.org; Sharma, Shashank
Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

On Mon, Jan 13, 2014 at 12:21:52PM +0530, Ramalingam C wrote:
> On slow HDMI hotplug out, because of the physical interface design, 
> DDC remains active for short duration even when HPD live status is 
> indicating the disconnect state. Because of this on VLV and HSW, slow 
> hotplug out events are not captured.
> 
> Hence this change uses the HPD pins live status to identify the HDMI 
> connector status.
> 
> Multiple forced detect and read modes calls come from user space for 
> different connectors, which can be handled with connector status and 
> previous detect event's cached EDID.
> With this approach for hot pluggable displays, EDID retrieval is 
> required only when there is a real hot-plug events.
> 
> This change also includes:
> 1. A logic to optimize those multiple calls, by re-using
>    cached data from previous detect and get_mode calls.
> 2. Store HDMI EDID, and re-use it in read modes.
> 3. Read live status reg to suppress spurious interrupts
> 
> These two changes will optimize the access to the DDC and also the CPU 
> cycles burned by intel_hdmi_detect and intel_hdmi_get_modes.
> 
> Ramalingam C (1):
>   drm/i915: HDMI detection based on HPD pin live status
> 
> Shashank Sharma (1):
>   drm/i915: Optimize EDID retrival on detect and get_modes
> 
>  drivers/gpu/drm/i915/intel_drv.h  |   12 +++
>  drivers/gpu/drm/i915/intel_hdmi.c |  149 
> ++++++++++++++++++++++++++++++++-----
>  2 files changed, 144 insertions(+), 17 deletions(-)

Nak.

We've had code to use the hpd live status register on all platforms, but had to take it out again on all platforms due to broken hardware: The live status simply doesn't work sometimes, resulting in angry users reporting black screens. So as-is patch 1 can't go into upstream. I know that it'd be really useful if we could rely on the hpd live status but, but thus far I couldn't come up with a good idea to make it work. Git history should have all the details.

Also note that machines with noisy interrupt lines (see the interrupt storm handling code in i915_irq.c) complicate this further.

This means that patch 2 is also a no-go since we really can't rely upon the hpd stuff for hdmi detection. But we can save EDID caching if we automatically invalidate the cached edid after 1-2 seconds or the next hpd interrupt (whichever is first). 1-2 seconds should be long enough for userspace to read the EDID a few times and change the output configuration, but not long enough for users to get pissed. Note that the caching interval should be shorter than the polling interval, which we use as a fallback if the hpd lines are noise and atm is at 10 seconds.

Also this EDID caching and invalidation code should imo be a generic helper with data structures (for the invalidation work and cached edid) in struct intel_output. That way we can also wire it up for e.g. VGA or any other output that'll benefit from EDID caching.

Aside: On DP the hpd pins seem to be reliable thus far, but I'm not sure whether that's just lack of test coverage (since DP screens are less
common) or because it actually works ...

Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Daniel Vetter]

On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank
<shashank.sharma@intel.com> wrote:
> Thanks a lot for your time, for reviewing the changes, and giving us some pointers.
> Both me and Ramalingam are designing this together, and we discussed about these changes and your suggestions.
> There are few things we would like to discuss about. Please correct us if some of our understanding is not proper.

First something I've forgotten in the original mail: Overall your
patches look really nice and the commit messages and cover letter have
been excellent. Unfortunately you've run into one of the nastier cases
of "reality just wont agree with the spec" :(

> Those two patches provide two solution.
> 1. Support for soft HPD, and slow removal of HDMI (when the DDC channel can still get the EDID).
> 2. Try to reduce the EDID reads over DDC channel for get connector and fill mode calls, by caching EDID, and using it until next HPD comes.
>
> Patch 2: Reduce the EDID read over DDC channel
> We are caching the EDID at every HPD up, on HDMI detect calls, and we are freeing it on subsequent HDMI disconnect calls.
>
> The design philosophy here is, to maintain a state machine of HDMI connector status, and differentiate between IOCTL detect calls and HPD detect calls.
> If there is a detect() or get_modes() call due to any of the IOCTL, which makes sure that input variable force=1, we just use the cached EDID, to serve this calls.
> But if the detect call is coming from HPD work function, due to a HPD plug-out, we remove/invalidate the old cached EDID, and cache the new EDID, on subsequent HDMI plug-in.
> From here, the same state machine follows.
>
> Can you please let us know, why do you think that we should invalidate the cached EDID after 1-2 seconds ?

Because there are machines out there where hpd never happens. So if
you keep onto the cached value forever userspace will never notice a
change in output configuration. Of course hotplug handling won't work,
but at least users can still manually probe outputs. By
unconditionally using the cached edid from ioctls you break this use
case. Yes, such machines are broken, but we need to keep them working
anyway.

Also in my experience all machines are affected, we have examples
covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt yet
since we don't rely on the hpd bits any more (and so won't see bug
reports any more).

Generally if you use the hpd stuff your code must be designed under
the assumption that hpd is completely unreliably. We've seen anything
from random noise, flat-out not-working at all, stuck bits and
unstable hpd values that occasionally flip-flop. So you can't rely on
it at all.

> Note: In this same patch, there is additional optimization, which you pointed out, where we check if the connector->status is same as live status.
> This can be removed independently, as you suggested.

Hm, where have I pointed this out? Some other mail on internal discussions?

> About patch 1:
> We have done some local experiments and we came to know that for VLV and HSW boards, we can rely on the live status, if we give it some time to settle (~300ms).
> Probably, we need to modify this patch, as you suggested, until it becomes handy to be used reliably. We are on it, and will send another patch soon.
>
> But if somehow we are able to get some consistent results from live status, do you think it would be worth accepting this change, so that it can handle soft HPDs and automation testing.
> Because I believe we will face this problem whenever we are trying to test something from automation, where the physical device is not removed, and DDC channel is up always.

It's very well possible that all the platforms you have, but
experience says that some OEM will horrible screw this up. At least
they've consistently botched this in the past on occasional machines.

Now the ghost hdmi detection on slow removal is obviously not great,
but we can't use the hpd bits to fix this. One approach would be.
1. Upon hpd interrupt do an immediate probe of the connector. This way
we'll have good userspace experience if the unplug happens quickly and
the hw works.
2. Re-probe with a 1s delay to catch slow-uplugs. The current output
probing helpers are clever enough already that if a state-change
happens to be detected a uevent will be generate, irrespective of the
source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).

Note that we already track the hpd interrupts on a per-source basis,
so doing the re-poll shouldn't be costly. Maybe do the re-poll as part
of the EDID invalidation to avoid stalling userspace.

But you can't rely upon the hpd pins unfortunately :(

This way we should be able to implement the 2 features you want, even
on unreliable hw.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Sharma, Shashank]

Thanks again for this explanation Daniel. 
We will work on your suggestions and come up with a new patch. 

Regards
Shashank  / Ramalingam
-----Original Message-----
From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Monday, January 13, 2014 6:57 PM
To: Sharma, Shashank
Cc: C, Ramalingam; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> Thanks a lot for your time, for reviewing the changes, and giving us some pointers.
> Both me and Ramalingam are designing this together, and we discussed about these changes and your suggestions.
> There are few things we would like to discuss about. Please correct us if some of our understanding is not proper.

First something I've forgotten in the original mail: Overall your patches look really nice and the commit messages and cover letter have been excellent. Unfortunately you've run into one of the nastier cases of "reality just wont agree with the spec" :(

> Those two patches provide two solution.
> 1. Support for soft HPD, and slow removal of HDMI (when the DDC channel can still get the EDID).
> 2. Try to reduce the EDID reads over DDC channel for get connector and fill mode calls, by caching EDID, and using it until next HPD comes.
>
> Patch 2: Reduce the EDID read over DDC channel We are caching the EDID 
> at every HPD up, on HDMI detect calls, and we are freeing it on subsequent HDMI disconnect calls.
>
> The design philosophy here is, to maintain a state machine of HDMI connector status, and differentiate between IOCTL detect calls and HPD detect calls.
> If there is a detect() or get_modes() call due to any of the IOCTL, which makes sure that input variable force=1, we just use the cached EDID, to serve this calls.
> But if the detect call is coming from HPD work function, due to a HPD plug-out, we remove/invalidate the old cached EDID, and cache the new EDID, on subsequent HDMI plug-in.
> From here, the same state machine follows.
>
> Can you please let us know, why do you think that we should invalidate the cached EDID after 1-2 seconds ?

Because there are machines out there where hpd never happens. So if you keep onto the cached value forever userspace will never notice a change in output configuration. Of course hotplug handling won't work, but at least users can still manually probe outputs. By unconditionally using the cached edid from ioctls you break this use case. Yes, such machines are broken, but we need to keep them working anyway.

Also in my experience all machines are affected, we have examples covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt yet since we don't rely on the hpd bits any more (and so won't see bug reports any more).

Generally if you use the hpd stuff your code must be designed under the assumption that hpd is completely unreliably. We've seen anything from random noise, flat-out not-working at all, stuck bits and unstable hpd values that occasionally flip-flop. So you can't rely on it at all.

> Note: In this same patch, there is additional optimization, which you pointed out, where we check if the connector->status is same as live status.
> This can be removed independently, as you suggested.

Hm, where have I pointed this out? Some other mail on internal discussions?

> About patch 1:
> We have done some local experiments and we came to know that for VLV and HSW boards, we can rely on the live status, if we give it some time to settle (~300ms).
> Probably, we need to modify this patch, as you suggested, until it becomes handy to be used reliably. We are on it, and will send another patch soon.
>
> But if somehow we are able to get some consistent results from live status, do you think it would be worth accepting this change, so that it can handle soft HPDs and automation testing.
> Because I believe we will face this problem whenever we are trying to test something from automation, where the physical device is not removed, and DDC channel is up always.

It's very well possible that all the platforms you have, but experience says that some OEM will horrible screw this up. At least they've consistently botched this in the past on occasional machines.

Now the ghost hdmi detection on slow removal is obviously not great, but we can't use the hpd bits to fix this. One approach would be.
1. Upon hpd interrupt do an immediate probe of the connector. This way we'll have good userspace experience if the unplug happens quickly and the hw works.
2. Re-probe with a 1s delay to catch slow-uplugs. The current output probing helpers are clever enough already that if a state-change happens to be detected a uevent will be generate, irrespective of the source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).

Note that we already track the hpd interrupts on a per-source basis, so doing the re-poll shouldn't be costly. Maybe do the re-poll as part of the EDID invalidation to avoid stalling userspace.

But you can't rely upon the hpd pins unfortunately :(

This way we should be able to implement the 2 features you want, even on unreliable hw.

Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Wang, Quanxian]

Hi, Sharma, Shashank

Is there any following patches to make it happen?

Thanks

Regards

Quanxian Wang

>-----Original Message-----
>From: intel-gfx-bounces@lists.freedesktop.org
>[mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, Shashank
>Sent: Tuesday, January 14, 2014 1:20 AM
>To: Daniel Vetter
>Cc: intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and
>get_modes
>
>Thanks again for this explanation Daniel.
>We will work on your suggestions and come up with a new patch.
>
>Regards
>Shashank  / Ramalingam
>-----Original Message-----
>From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
>Vetter
>Sent: Monday, January 13, 2014 6:57 PM
>To: Sharma, Shashank
>Cc: C, Ramalingam; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and
>get_modes
>
>On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank
><shashank.sharma@intel.com> wrote:
>> Thanks a lot for your time, for reviewing the changes, and giving us some
>pointers.
>> Both me and Ramalingam are designing this together, and we discussed about
>these changes and your suggestions.
>> There are few things we would like to discuss about. Please correct us if some of
>our understanding is not proper.
>
>First something I've forgotten in the original mail: Overall your patches look really
>nice and the commit messages and cover letter have been excellent.
>Unfortunately you've run into one of the nastier cases of "reality just wont agree
>with the spec" :(
>
>> Those two patches provide two solution.
>> 1. Support for soft HPD, and slow removal of HDMI (when the DDC channel can
>still get the EDID).
>> 2. Try to reduce the EDID reads over DDC channel for get connector and fill mode
>calls, by caching EDID, and using it until next HPD comes.
>>
>> Patch 2: Reduce the EDID read over DDC channel We are caching the EDID
>> at every HPD up, on HDMI detect calls, and we are freeing it on subsequent
>HDMI disconnect calls.
>>
>> The design philosophy here is, to maintain a state machine of HDMI connector
>status, and differentiate between IOCTL detect calls and HPD detect calls.
>> If there is a detect() or get_modes() call due to any of the IOCTL, which makes
>sure that input variable force=1, we just use the cached EDID, to serve this calls.
>> But if the detect call is coming from HPD work function, due to a HPD plug-out,
>we remove/invalidate the old cached EDID, and cache the new EDID, on
>subsequent HDMI plug-in.
>> From here, the same state machine follows.
>>
>> Can you please let us know, why do you think that we should invalidate the
>cached EDID after 1-2 seconds ?
>
>Because there are machines out there where hpd never happens. So if you keep
>onto the cached value forever userspace will never notice a change in output
>configuration. Of course hotplug handling won't work, but at least users can still
>manually probe outputs. By unconditionally using the cached edid from ioctls you
>break this use case. Yes, such machines are broken, but we need to keep them
>working anyway.
>
>Also in my experience all machines are affected, we have examples covering gm45,
>ilk, snb & ivb. We haven't seen a case for hsw/byt yet since we don't rely on the
>hpd bits any more (and so won't see bug reports any more).
>
>Generally if you use the hpd stuff your code must be designed under the
>assumption that hpd is completely unreliably. We've seen anything from random
>noise, flat-out not-working at all, stuck bits and unstable hpd values that
>occasionally flip-flop. So you can't rely on it at all.
>
>> Note: In this same patch, there is additional optimization, which you pointed out,
>where we check if the connector->status is same as live status.
>> This can be removed independently, as you suggested.
>
>Hm, where have I pointed this out? Some other mail on internal discussions?
>
>> About patch 1:
>> We have done some local experiments and we came to know that for VLV and
>HSW boards, we can rely on the live status, if we give it some time to settle
>(~300ms).
>> Probably, we need to modify this patch, as you suggested, until it becomes
>handy to be used reliably. We are on it, and will send another patch soon.
>>
>> But if somehow we are able to get some consistent results from live status, do
>you think it would be worth accepting this change, so that it can handle soft HPDs
>and automation testing.
>> Because I believe we will face this problem whenever we are trying to test
>something from automation, where the physical device is not removed, and DDC
>channel is up always.
>
>It's very well possible that all the platforms you have, but experience says that
>some OEM will horrible screw this up. At least they've consistently botched this in
>the past on occasional machines.
>
>Now the ghost hdmi detection on slow removal is obviously not great, but we
>can't use the hpd bits to fix this. One approach would be.
>1. Upon hpd interrupt do an immediate probe of the connector. This way we'll
>have good userspace experience if the unplug happens quickly and the hw works.
>2. Re-probe with a 1s delay to catch slow-uplugs. The current output probing
>helpers are clever enough already that if a state-change happens to be detected a
>uevent will be generate, irrespective of the source of the detect call (i.e. hpd,
>kernel poll or ioctl/sysfs).
>
>Note that we already track the hpd interrupts on a per-source basis, so doing the
>re-poll shouldn't be costly. Maybe do the re-poll as part of the EDID invalidation to
>avoid stalling userspace.
>
>But you can't rely upon the hpd pins unfortunately :(
>
>This way we should be able to implement the 2 features you want, even on
>unreliable hw.
>
>Cheers, Daniel
>--
>Daniel Vetter
>Software Engineer, Intel Corporation
>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Sharma, Shashank]

Hello Quanxian Wang

This patch is available and working on all MCG tree's (Main, R42B and R44B)
We were trying to opensource this patch, but due to the dependency on live_status reg, we had to change the design. 

I was working on that, but couldn't finish the activity yet, Thanks for reminding me 
I will update soon. :)

Regards
Shashank   
-----Original Message-----
From: Wang, Quanxian 
Sent: Wednesday, April 09, 2014 11:49 AM
To: Sharma, Shashank; Daniel Vetter
Cc: intel-gfx@lists.freedesktop.org
Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

Hi, Sharma, Shashank

Is there any following patches to make it happen?

Thanks

Regards

Quanxian Wang

>-----Original Message-----
>From: intel-gfx-bounces@lists.freedesktop.org
>[mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, 
>Shashank
>Sent: Tuesday, January 14, 2014 1:20 AM
>To: Daniel Vetter
>Cc: intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
>and get_modes
>
>Thanks again for this explanation Daniel.
>We will work on your suggestions and come up with a new patch.
>
>Regards
>Shashank  / Ramalingam
>-----Original Message-----
>From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf 
>Of Daniel Vetter
>Sent: Monday, January 13, 2014 6:57 PM
>To: Sharma, Shashank
>Cc: C, Ramalingam; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
>and get_modes
>
>On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank 
><shashank.sharma@intel.com> wrote:
>> Thanks a lot for your time, for reviewing the changes, and giving us 
>> some
>pointers.
>> Both me and Ramalingam are designing this together, and we discussed 
>> about
>these changes and your suggestions.
>> There are few things we would like to discuss about. Please correct 
>> us if some of
>our understanding is not proper.
>
>First something I've forgotten in the original mail: Overall your 
>patches look really nice and the commit messages and cover letter have been excellent.
>Unfortunately you've run into one of the nastier cases of "reality just 
>wont agree with the spec" :(
>
>> Those two patches provide two solution.
>> 1. Support for soft HPD, and slow removal of HDMI (when the DDC 
>> channel can
>still get the EDID).
>> 2. Try to reduce the EDID reads over DDC channel for get connector 
>> and fill mode
>calls, by caching EDID, and using it until next HPD comes.
>>
>> Patch 2: Reduce the EDID read over DDC channel We are caching the 
>> EDID at every HPD up, on HDMI detect calls, and we are freeing it on 
>> subsequent
>HDMI disconnect calls.
>>
>> The design philosophy here is, to maintain a state machine of HDMI 
>> connector
>status, and differentiate between IOCTL detect calls and HPD detect calls.
>> If there is a detect() or get_modes() call due to any of the IOCTL, 
>> which makes
>sure that input variable force=1, we just use the cached EDID, to serve this calls.
>> But if the detect call is coming from HPD work function, due to a HPD 
>> plug-out,
>we remove/invalidate the old cached EDID, and cache the new EDID, on 
>subsequent HDMI plug-in.
>> From here, the same state machine follows.
>>
>> Can you please let us know, why do you think that we should 
>> invalidate the
>cached EDID after 1-2 seconds ?
>
>Because there are machines out there where hpd never happens. So if you 
>keep onto the cached value forever userspace will never notice a change 
>in output configuration. Of course hotplug handling won't work, but at 
>least users can still manually probe outputs. By unconditionally using 
>the cached edid from ioctls you break this use case. Yes, such machines 
>are broken, but we need to keep them working anyway.
>
>Also in my experience all machines are affected, we have examples 
>covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt yet 
>since we don't rely on the hpd bits any more (and so won't see bug reports any more).
>
>Generally if you use the hpd stuff your code must be designed under the 
>assumption that hpd is completely unreliably. We've seen anything from 
>random noise, flat-out not-working at all, stuck bits and unstable hpd 
>values that occasionally flip-flop. So you can't rely on it at all.
>
>> Note: In this same patch, there is additional optimization, which you 
>> pointed out,
>where we check if the connector->status is same as live status.
>> This can be removed independently, as you suggested.
>
>Hm, where have I pointed this out? Some other mail on internal discussions?
>
>> About patch 1:
>> We have done some local experiments and we came to know that for VLV 
>> and
>HSW boards, we can rely on the live status, if we give it some time to 
>settle (~300ms).
>> Probably, we need to modify this patch, as you suggested, until it 
>> becomes
>handy to be used reliably. We are on it, and will send another patch soon.
>>
>> But if somehow we are able to get some consistent results from live 
>> status, do
>you think it would be worth accepting this change, so that it can 
>handle soft HPDs and automation testing.
>> Because I believe we will face this problem whenever we are trying to 
>> test
>something from automation, where the physical device is not removed, 
>and DDC channel is up always.
>
>It's very well possible that all the platforms you have, but experience 
>says that some OEM will horrible screw this up. At least they've 
>consistently botched this in the past on occasional machines.
>
>Now the ghost hdmi detection on slow removal is obviously not great, 
>but we can't use the hpd bits to fix this. One approach would be.
>1. Upon hpd interrupt do an immediate probe of the connector. This way 
>we'll have good userspace experience if the unplug happens quickly and the hw works.
>2. Re-probe with a 1s delay to catch slow-uplugs. The current output 
>probing helpers are clever enough already that if a state-change 
>happens to be detected a uevent will be generate, irrespective of the 
>source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).
>
>Note that we already track the hpd interrupts on a per-source basis, so 
>doing the re-poll shouldn't be costly. Maybe do the re-poll as part of 
>the EDID invalidation to avoid stalling userspace.
>
>But you can't rely upon the hpd pins unfortunately :(
>
>This way we should be able to implement the 2 features you want, even 
>on unreliable hw.
>
>Cheers, Daniel
>--
>Daniel Vetter
>Software Engineer, Intel Corporation
>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Sharma, Shashank]

Hi Daniel / Quanxian / All, 

I have one question about the 'force' flag given to connector's detect() functions.
To design new EDID caching solution, I was trying to re-use this flag. As you all know, detect() gets called from few places in DRM and I915 layer, with this flag status:

drm_sysfs.c : status_show (sysfs status check inquire from USP)						force = 1
drm_crtc_helper.c: drm_helper_probe_single_connector_modes (part of get_connector IOCTL) 		force = 1

drm_crtc_helper.c: output_poll_execute(scheduled from poll work)						force = 0
drm_crtc_helper.c: drm_helper_hpd_irq_event (resume hotplug)						force = 0
i915_irq.c: i915_hotplug_work_function (bottom half of hotplug IRQ)						force = 0

What I am seeing is, the places where it's really required to probe the device, like in IRQ handlers or while resuming the device,  the force flag is 0, 
whereas whenever there is a userspace interaction or query for status, the flag is 1.  Please correct me if my understating is not proper, 
but I feel this should be the opposite way. 

Please let me know your opinion about this.

Regards
Shashank  
-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, Shashank
Sent: Wednesday, April 09, 2014 12:20 PM
To: Wang, Quanxian; Daniel Vetter
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

Hello Quanxian Wang

This patch is available and working on all MCG tree's (Main, R42B and R44B) We were trying to opensource this patch, but due to the dependency on live_status reg, we had to change the design. 

I was working on that, but couldn't finish the activity yet, Thanks for reminding me I will update soon. :)

Regards
Shashank   
-----Original Message-----
From: Wang, Quanxian
Sent: Wednesday, April 09, 2014 11:49 AM
To: Sharma, Shashank; Daniel Vetter
Cc: intel-gfx@lists.freedesktop.org
Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

Hi, Sharma, Shashank

Is there any following patches to make it happen?

Thanks

Regards

Quanxian Wang

>-----Original Message-----
>From: intel-gfx-bounces@lists.freedesktop.org
>[mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, 
>Shashank
>Sent: Tuesday, January 14, 2014 1:20 AM
>To: Daniel Vetter
>Cc: intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
>and get_modes
>
>Thanks again for this explanation Daniel.
>We will work on your suggestions and come up with a new patch.
>
>Regards
>Shashank  / Ramalingam
>-----Original Message-----
>From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf 
>Of Daniel Vetter
>Sent: Monday, January 13, 2014 6:57 PM
>To: Sharma, Shashank
>Cc: C, Ramalingam; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
>and get_modes
>
>On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank 
><shashank.sharma@intel.com> wrote:
>> Thanks a lot for your time, for reviewing the changes, and giving us 
>> some
>pointers.
>> Both me and Ramalingam are designing this together, and we discussed 
>> about
>these changes and your suggestions.
>> There are few things we would like to discuss about. Please correct 
>> us if some of
>our understanding is not proper.
>
>First something I've forgotten in the original mail: Overall your 
>patches look really nice and the commit messages and cover letter have been excellent.
>Unfortunately you've run into one of the nastier cases of "reality just 
>wont agree with the spec" :(
>
>> Those two patches provide two solution.
>> 1. Support for soft HPD, and slow removal of HDMI (when the DDC 
>> channel can
>still get the EDID).
>> 2. Try to reduce the EDID reads over DDC channel for get connector 
>> and fill mode
>calls, by caching EDID, and using it until next HPD comes.
>>
>> Patch 2: Reduce the EDID read over DDC channel We are caching the 
>> EDID at every HPD up, on HDMI detect calls, and we are freeing it on 
>> subsequent
>HDMI disconnect calls.
>>
>> The design philosophy here is, to maintain a state machine of HDMI 
>> connector
>status, and differentiate between IOCTL detect calls and HPD detect calls.
>> If there is a detect() or get_modes() call due to any of the IOCTL, 
>> which makes
>sure that input variable force=1, we just use the cached EDID, to serve this calls.
>> But if the detect call is coming from HPD work function, due to a HPD 
>> plug-out,
>we remove/invalidate the old cached EDID, and cache the new EDID, on 
>subsequent HDMI plug-in.
>> From here, the same state machine follows.
>>
>> Can you please let us know, why do you think that we should 
>> invalidate the
>cached EDID after 1-2 seconds ?
>
>Because there are machines out there where hpd never happens. So if you 
>keep onto the cached value forever userspace will never notice a change 
>in output configuration. Of course hotplug handling won't work, but at 
>least users can still manually probe outputs. By unconditionally using 
>the cached edid from ioctls you break this use case. Yes, such machines 
>are broken, but we need to keep them working anyway.
>
>Also in my experience all machines are affected, we have examples 
>covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt yet 
>since we don't rely on the hpd bits any more (and so won't see bug reports any more).
>
>Generally if you use the hpd stuff your code must be designed under the 
>assumption that hpd is completely unreliably. We've seen anything from 
>random noise, flat-out not-working at all, stuck bits and unstable hpd 
>values that occasionally flip-flop. So you can't rely on it at all.
>
>> Note: In this same patch, there is additional optimization, which you 
>> pointed out,
>where we check if the connector->status is same as live status.
>> This can be removed independently, as you suggested.
>
>Hm, where have I pointed this out? Some other mail on internal discussions?
>
>> About patch 1:
>> We have done some local experiments and we came to know that for VLV 
>> and
>HSW boards, we can rely on the live status, if we give it some time to 
>settle (~300ms).
>> Probably, we need to modify this patch, as you suggested, until it 
>> becomes
>handy to be used reliably. We are on it, and will send another patch soon.
>>
>> But if somehow we are able to get some consistent results from live 
>> status, do
>you think it would be worth accepting this change, so that it can 
>handle soft HPDs and automation testing.
>> Because I believe we will face this problem whenever we are trying to 
>> test
>something from automation, where the physical device is not removed, 
>and DDC channel is up always.
>
>It's very well possible that all the platforms you have, but experience 
>says that some OEM will horrible screw this up. At least they've 
>consistently botched this in the past on occasional machines.
>
>Now the ghost hdmi detection on slow removal is obviously not great, 
>but we can't use the hpd bits to fix this. One approach would be.
>1. Upon hpd interrupt do an immediate probe of the connector. This way 
>we'll have good userspace experience if the unplug happens quickly and the hw works.
>2. Re-probe with a 1s delay to catch slow-uplugs. The current output 
>probing helpers are clever enough already that if a state-change 
>happens to be detected a uevent will be generate, irrespective of the 
>source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).
>
>Note that we already track the hpd interrupts on a per-source basis, so 
>doing the re-poll shouldn't be costly. Maybe do the re-poll as part of 
>the EDID invalidation to avoid stalling userspace.
>
>But you can't rely upon the hpd pins unfortunately :(
>
>This way we should be able to implement the 2 features you want, even 
>on unreliable hw.
>
>Cheers, Daniel
>--
>Daniel Vetter
>Software Engineer, Intel Corporation
>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
>_______________________________________________
>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

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Daniel Vetter]

On Thu, Apr 10, 2014 at 06:46:58AM +0000, Sharma, Shashank wrote:
> Hi Daniel / Quanxian / All, 
> 
> I have one question about the 'force' flag given to connector's detect() functions.
> To design new EDID caching solution, I was trying to re-use this flag.
> As you all know, detect() gets called from few places in DRM and I915
> layer, with this flag status:
> 
> drm_sysfs.c : status_show (sysfs status check inquire from USP)						force = 1
> drm_crtc_helper.c: drm_helper_probe_single_connector_modes (part of get_connector IOCTL) 		force = 1
> 
> drm_crtc_helper.c: output_poll_execute(scheduled from poll work)						force = 0
> drm_crtc_helper.c: drm_helper_hpd_irq_event (resume hotplug)						force = 0
> i915_irq.c: i915_hotplug_work_function (bottom half of hotplug IRQ)						force = 0
> 
> What I am seeing is, the places where it's really required to probe the
> device, like in IRQ handlers or while resuming the device,  the force
> flag is 0, whereas whenever there is a userspace interaction or query
> for status, the flag is 1.  Please correct me if my understating is not
> proper, 
> but I feel this should be the opposite way. 
> 
> Please let me know your opinion about this.

Force is a bit misnomer, a better name might be non-invasive. Without
force we don't do stuff like load-detect by default since at least when
doing this manually some old crt screens switch on. But this is really
only relevant for gen2/3 and tv-out, so not of any concern on modern
platforms. Essentially force means "userspace asked for this and it's ok
if the screen flickers a bit due to that".

Imo you can do the caching and use the cached version irrespective of
force.
-Daniel

> 
> Regards
> Shashank  
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, Shashank
> Sent: Wednesday, April 09, 2014 12:20 PM
> To: Wang, Quanxian; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
> 
> Hello Quanxian Wang
> 
> This patch is available and working on all MCG tree's (Main, R42B and R44B) We were trying to opensource this patch, but due to the dependency on live_status reg, we had to change the design. 
> 
> I was working on that, but couldn't finish the activity yet, Thanks for reminding me I will update soon. :)
> 
> Regards
> Shashank   
> -----Original Message-----
> From: Wang, Quanxian
> Sent: Wednesday, April 09, 2014 11:49 AM
> To: Sharma, Shashank; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
> 
> Hi, Sharma, Shashank
> 
> Is there any following patches to make it happen?
> 
> Thanks
> 
> Regards
> 
> Quanxian Wang
> 
> >-----Original Message-----
> >From: intel-gfx-bounces@lists.freedesktop.org
> >[mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, 
> >Shashank
> >Sent: Tuesday, January 14, 2014 1:20 AM
> >To: Daniel Vetter
> >Cc: intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> >and get_modes
> >
> >Thanks again for this explanation Daniel.
> >We will work on your suggestions and come up with a new patch.
> >
> >Regards
> >Shashank  / Ramalingam
> >-----Original Message-----
> >From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf 
> >Of Daniel Vetter
> >Sent: Monday, January 13, 2014 6:57 PM
> >To: Sharma, Shashank
> >Cc: C, Ramalingam; intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> >and get_modes
> >
> >On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank 
> ><shashank.sharma@intel.com> wrote:
> >> Thanks a lot for your time, for reviewing the changes, and giving us 
> >> some
> >pointers.
> >> Both me and Ramalingam are designing this together, and we discussed 
> >> about
> >these changes and your suggestions.
> >> There are few things we would like to discuss about. Please correct 
> >> us if some of
> >our understanding is not proper.
> >
> >First something I've forgotten in the original mail: Overall your 
> >patches look really nice and the commit messages and cover letter have been excellent.
> >Unfortunately you've run into one of the nastier cases of "reality just 
> >wont agree with the spec" :(
> >
> >> Those two patches provide two solution.
> >> 1. Support for soft HPD, and slow removal of HDMI (when the DDC 
> >> channel can
> >still get the EDID).
> >> 2. Try to reduce the EDID reads over DDC channel for get connector 
> >> and fill mode
> >calls, by caching EDID, and using it until next HPD comes.
> >>
> >> Patch 2: Reduce the EDID read over DDC channel We are caching the 
> >> EDID at every HPD up, on HDMI detect calls, and we are freeing it on 
> >> subsequent
> >HDMI disconnect calls.
> >>
> >> The design philosophy here is, to maintain a state machine of HDMI 
> >> connector
> >status, and differentiate between IOCTL detect calls and HPD detect calls.
> >> If there is a detect() or get_modes() call due to any of the IOCTL, 
> >> which makes
> >sure that input variable force=1, we just use the cached EDID, to serve this calls.
> >> But if the detect call is coming from HPD work function, due to a HPD 
> >> plug-out,
> >we remove/invalidate the old cached EDID, and cache the new EDID, on 
> >subsequent HDMI plug-in.
> >> From here, the same state machine follows.
> >>
> >> Can you please let us know, why do you think that we should 
> >> invalidate the
> >cached EDID after 1-2 seconds ?
> >
> >Because there are machines out there where hpd never happens. So if you 
> >keep onto the cached value forever userspace will never notice a change 
> >in output configuration. Of course hotplug handling won't work, but at 
> >least users can still manually probe outputs. By unconditionally using 
> >the cached edid from ioctls you break this use case. Yes, such machines 
> >are broken, but we need to keep them working anyway.
> >
> >Also in my experience all machines are affected, we have examples 
> >covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt yet 
> >since we don't rely on the hpd bits any more (and so won't see bug reports any more).
> >
> >Generally if you use the hpd stuff your code must be designed under the 
> >assumption that hpd is completely unreliably. We've seen anything from 
> >random noise, flat-out not-working at all, stuck bits and unstable hpd 
> >values that occasionally flip-flop. So you can't rely on it at all.
> >
> >> Note: In this same patch, there is additional optimization, which you 
> >> pointed out,
> >where we check if the connector->status is same as live status.
> >> This can be removed independently, as you suggested.
> >
> >Hm, where have I pointed this out? Some other mail on internal discussions?
> >
> >> About patch 1:
> >> We have done some local experiments and we came to know that for VLV 
> >> and
> >HSW boards, we can rely on the live status, if we give it some time to 
> >settle (~300ms).
> >> Probably, we need to modify this patch, as you suggested, until it 
> >> becomes
> >handy to be used reliably. We are on it, and will send another patch soon.
> >>
> >> But if somehow we are able to get some consistent results from live 
> >> status, do
> >you think it would be worth accepting this change, so that it can 
> >handle soft HPDs and automation testing.
> >> Because I believe we will face this problem whenever we are trying to 
> >> test
> >something from automation, where the physical device is not removed, 
> >and DDC channel is up always.
> >
> >It's very well possible that all the platforms you have, but experience 
> >says that some OEM will horrible screw this up. At least they've 
> >consistently botched this in the past on occasional machines.
> >
> >Now the ghost hdmi detection on slow removal is obviously not great, 
> >but we can't use the hpd bits to fix this. One approach would be.
> >1. Upon hpd interrupt do an immediate probe of the connector. This way 
> >we'll have good userspace experience if the unplug happens quickly and the hw works.
> >2. Re-probe with a 1s delay to catch slow-uplugs. The current output 
> >probing helpers are clever enough already that if a state-change 
> >happens to be detected a uevent will be generate, irrespective of the 
> >source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).
> >
> >Note that we already track the hpd interrupts on a per-source basis, so 
> >doing the re-poll shouldn't be costly. Maybe do the re-poll as part of 
> >the EDID invalidation to avoid stalling userspace.
> >
> >But you can't rely upon the hpd pins unfortunately :(
> >
> >This way we should be able to implement the 2 features you want, even 
> >on unreliable hw.
> >
> >Cheers, Daniel
> >--
> >Daniel Vetter
> >Software Engineer, Intel Corporation
> >+41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >_______________________________________________
> >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
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Sharma, Shashank]

Thanks for this clarification. 

Regards
Shashank 
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Thursday, April 10, 2014 1:39 PM
To: Sharma, Shashank
Cc: Wang, Quanxian; Daniel Vetter; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

On Thu, Apr 10, 2014 at 06:46:58AM +0000, Sharma, Shashank wrote:
> Hi Daniel / Quanxian / All,
> 
> I have one question about the 'force' flag given to connector's detect() functions.
> To design new EDID caching solution, I was trying to re-use this flag.
> As you all know, detect() gets called from few places in DRM and I915 
> layer, with this flag status:
> 
> drm_sysfs.c : status_show (sysfs status check inquire from USP)						force = 1
> drm_crtc_helper.c: drm_helper_probe_single_connector_modes (part of get_connector IOCTL) 		force = 1
> 
> drm_crtc_helper.c: output_poll_execute(scheduled from poll work)						force = 0
> drm_crtc_helper.c: drm_helper_hpd_irq_event (resume hotplug)						force = 0
> i915_irq.c: i915_hotplug_work_function (bottom half of hotplug IRQ)						force = 0
> 
> What I am seeing is, the places where it's really required to probe 
> the device, like in IRQ handlers or while resuming the device,  the 
> force flag is 0, whereas whenever there is a userspace interaction or 
> query for status, the flag is 1.  Please correct me if my understating 
> is not proper, but I feel this should be the opposite way.
> 
> Please let me know your opinion about this.

Force is a bit misnomer, a better name might be non-invasive. Without force we don't do stuff like load-detect by default since at least when doing this manually some old crt screens switch on. But this is really only relevant for gen2/3 and tv-out, so not of any concern on modern platforms. Essentially force means "userspace asked for this and it's ok if the screen flickers a bit due to that".

Imo you can do the caching and use the cached version irrespective of force.
-Daniel

> 
> Regards
> Shashank
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
> Behalf Of Sharma, Shashank
> Sent: Wednesday, April 09, 2014 12:20 PM
> To: Wang, Quanxian; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> and get_modes
> 
> Hello Quanxian Wang
> 
> This patch is available and working on all MCG tree's (Main, R42B and R44B) We were trying to opensource this patch, but due to the dependency on live_status reg, we had to change the design. 
> 
> I was working on that, but couldn't finish the activity yet, Thanks 
> for reminding me I will update soon. :)
> 
> Regards
> Shashank   
> -----Original Message-----
> From: Wang, Quanxian
> Sent: Wednesday, April 09, 2014 11:49 AM
> To: Sharma, Shashank; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> and get_modes
> 
> Hi, Sharma, Shashank
> 
> Is there any following patches to make it happen?
> 
> Thanks
> 
> Regards
> 
> Quanxian Wang
> 
> >-----Original Message-----
> >From: intel-gfx-bounces@lists.freedesktop.org
> >[mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, 
> >Shashank
> >Sent: Tuesday, January 14, 2014 1:20 AM
> >To: Daniel Vetter
> >Cc: intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI 
> >detect and get_modes
> >
> >Thanks again for this explanation Daniel.
> >We will work on your suggestions and come up with a new patch.
> >
> >Regards
> >Shashank  / Ramalingam
> >-----Original Message-----
> >From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On 
> >Behalf Of Daniel Vetter
> >Sent: Monday, January 13, 2014 6:57 PM
> >To: Sharma, Shashank
> >Cc: C, Ramalingam; intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI 
> >detect and get_modes
> >
> >On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank 
> ><shashank.sharma@intel.com> wrote:
> >> Thanks a lot for your time, for reviewing the changes, and giving 
> >> us some
> >pointers.
> >> Both me and Ramalingam are designing this together, and we 
> >> discussed about
> >these changes and your suggestions.
> >> There are few things we would like to discuss about. Please correct 
> >> us if some of
> >our understanding is not proper.
> >
> >First something I've forgotten in the original mail: Overall your 
> >patches look really nice and the commit messages and cover letter have been excellent.
> >Unfortunately you've run into one of the nastier cases of "reality 
> >just wont agree with the spec" :(
> >
> >> Those two patches provide two solution.
> >> 1. Support for soft HPD, and slow removal of HDMI (when the DDC 
> >> channel can
> >still get the EDID).
> >> 2. Try to reduce the EDID reads over DDC channel for get connector 
> >> and fill mode
> >calls, by caching EDID, and using it until next HPD comes.
> >>
> >> Patch 2: Reduce the EDID read over DDC channel We are caching the 
> >> EDID at every HPD up, on HDMI detect calls, and we are freeing it 
> >> on subsequent
> >HDMI disconnect calls.
> >>
> >> The design philosophy here is, to maintain a state machine of HDMI 
> >> connector
> >status, and differentiate between IOCTL detect calls and HPD detect calls.
> >> If there is a detect() or get_modes() call due to any of the IOCTL, 
> >> which makes
> >sure that input variable force=1, we just use the cached EDID, to serve this calls.
> >> But if the detect call is coming from HPD work function, due to a 
> >> HPD plug-out,
> >we remove/invalidate the old cached EDID, and cache the new EDID, on 
> >subsequent HDMI plug-in.
> >> From here, the same state machine follows.
> >>
> >> Can you please let us know, why do you think that we should 
> >> invalidate the
> >cached EDID after 1-2 seconds ?
> >
> >Because there are machines out there where hpd never happens. So if 
> >you keep onto the cached value forever userspace will never notice a 
> >change in output configuration. Of course hotplug handling won't 
> >work, but at least users can still manually probe outputs. By 
> >unconditionally using the cached edid from ioctls you break this use 
> >case. Yes, such machines are broken, but we need to keep them working anyway.
> >
> >Also in my experience all machines are affected, we have examples 
> >covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt yet 
> >since we don't rely on the hpd bits any more (and so won't see bug reports any more).
> >
> >Generally if you use the hpd stuff your code must be designed under 
> >the assumption that hpd is completely unreliably. We've seen anything 
> >from random noise, flat-out not-working at all, stuck bits and 
> >unstable hpd values that occasionally flip-flop. So you can't rely on it at all.
> >
> >> Note: In this same patch, there is additional optimization, which 
> >> you pointed out,
> >where we check if the connector->status is same as live status.
> >> This can be removed independently, as you suggested.
> >
> >Hm, where have I pointed this out? Some other mail on internal discussions?
> >
> >> About patch 1:
> >> We have done some local experiments and we came to know that for 
> >> VLV and
> >HSW boards, we can rely on the live status, if we give it some time 
> >to settle (~300ms).
> >> Probably, we need to modify this patch, as you suggested, until it 
> >> becomes
> >handy to be used reliably. We are on it, and will send another patch soon.
> >>
> >> But if somehow we are able to get some consistent results from live 
> >> status, do
> >you think it would be worth accepting this change, so that it can 
> >handle soft HPDs and automation testing.
> >> Because I believe we will face this problem whenever we are trying 
> >> to test
> >something from automation, where the physical device is not removed, 
> >and DDC channel is up always.
> >
> >It's very well possible that all the platforms you have, but 
> >experience says that some OEM will horrible screw this up. At least 
> >they've consistently botched this in the past on occasional machines.
> >
> >Now the ghost hdmi detection on slow removal is obviously not great, 
> >but we can't use the hpd bits to fix this. One approach would be.
> >1. Upon hpd interrupt do an immediate probe of the connector. This 
> >way we'll have good userspace experience if the unplug happens quickly and the hw works.
> >2. Re-probe with a 1s delay to catch slow-uplugs. The current output 
> >probing helpers are clever enough already that if a state-change 
> >happens to be detected a uevent will be generate, irrespective of the 
> >source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).
> >
> >Note that we already track the hpd interrupts on a per-source basis, 
> >so doing the re-poll shouldn't be costly. Maybe do the re-poll as 
> >part of the EDID invalidation to avoid stalling userspace.
> >
> >But you can't rely upon the hpd pins unfortunately :(
> >
> >This way we should be able to implement the 2 features you want, even 
> >on unreliable hw.
> >
> >Cheers, Daniel
> >--
> >Daniel Vetter
> >Software Engineer, Intel Corporation
> >+41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >_______________________________________________
> >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
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Wang, Quanxian]

Tizen-IVI has one feature will require hdmi edid cache. It will be happy to find your patch pushed into upstream tree.

Regards

Thanks

Quanxian Wang

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, Shashank
Sent: Wednesday, April 09, 2014 12:20 PM
To: Wang, Quanxian; Daniel Vetter
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

Hello Quanxian Wang

This patch is available and working on all MCG tree's (Main, R42B and R44B) We were trying to opensource this patch, but due to the dependency on live_status reg, we had to change the design. 

I was working on that, but couldn't finish the activity yet, Thanks for reminding me I will update soon. :)

Regards
Shashank   
-----Original Message-----
From: Wang, Quanxian
Sent: Wednesday, April 09, 2014 11:49 AM
To: Sharma, Shashank; Daniel Vetter
Cc: intel-gfx@lists.freedesktop.org
Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

Hi, Sharma, Shashank

Is there any following patches to make it happen?

Thanks

Regards

Quanxian Wang

>-----Original Message-----
>From: intel-gfx-bounces@lists.freedesktop.org
>[mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, 
>Shashank
>Sent: Tuesday, January 14, 2014 1:20 AM
>To: Daniel Vetter
>Cc: intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
>and get_modes
>
>Thanks again for this explanation Daniel.
>We will work on your suggestions and come up with a new patch.
>
>Regards
>Shashank  / Ramalingam
>-----Original Message-----
>From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf 
>Of Daniel Vetter
>Sent: Monday, January 13, 2014 6:57 PM
>To: Sharma, Shashank
>Cc: C, Ramalingam; intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
>and get_modes
>
>On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank 
><shashank.sharma@intel.com> wrote:
>> Thanks a lot for your time, for reviewing the changes, and giving us 
>> some
>pointers.
>> Both me and Ramalingam are designing this together, and we discussed 
>> about
>these changes and your suggestions.
>> There are few things we would like to discuss about. Please correct 
>> us if some of
>our understanding is not proper.
>
>First something I've forgotten in the original mail: Overall your 
>patches look really nice and the commit messages and cover letter have been excellent.
>Unfortunately you've run into one of the nastier cases of "reality just 
>wont agree with the spec" :(
>
>> Those two patches provide two solution.
>> 1. Support for soft HPD, and slow removal of HDMI (when the DDC 
>> channel can
>still get the EDID).
>> 2. Try to reduce the EDID reads over DDC channel for get connector 
>> and fill mode
>calls, by caching EDID, and using it until next HPD comes.
>>
>> Patch 2: Reduce the EDID read over DDC channel We are caching the 
>> EDID at every HPD up, on HDMI detect calls, and we are freeing it on 
>> subsequent
>HDMI disconnect calls.
>>
>> The design philosophy here is, to maintain a state machine of HDMI 
>> connector
>status, and differentiate between IOCTL detect calls and HPD detect calls.
>> If there is a detect() or get_modes() call due to any of the IOCTL, 
>> which makes
>sure that input variable force=1, we just use the cached EDID, to serve this calls.
>> But if the detect call is coming from HPD work function, due to a HPD 
>> plug-out,
>we remove/invalidate the old cached EDID, and cache the new EDID, on 
>subsequent HDMI plug-in.
>> From here, the same state machine follows.
>>
>> Can you please let us know, why do you think that we should 
>> invalidate the
>cached EDID after 1-2 seconds ?
>
>Because there are machines out there where hpd never happens. So if you 
>keep onto the cached value forever userspace will never notice a change 
>in output configuration. Of course hotplug handling won't work, but at 
>least users can still manually probe outputs. By unconditionally using 
>the cached edid from ioctls you break this use case. Yes, such machines 
>are broken, but we need to keep them working anyway.
>
>Also in my experience all machines are affected, we have examples 
>covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt yet 
>since we don't rely on the hpd bits any more (and so won't see bug reports any more).
>
>Generally if you use the hpd stuff your code must be designed under the 
>assumption that hpd is completely unreliably. We've seen anything from 
>random noise, flat-out not-working at all, stuck bits and unstable hpd 
>values that occasionally flip-flop. So you can't rely on it at all.
>
>> Note: In this same patch, there is additional optimization, which you 
>> pointed out,
>where we check if the connector->status is same as live status.
>> This can be removed independently, as you suggested.
>
>Hm, where have I pointed this out? Some other mail on internal discussions?
>
>> About patch 1:
>> We have done some local experiments and we came to know that for VLV 
>> and
>HSW boards, we can rely on the live status, if we give it some time to 
>settle (~300ms).
>> Probably, we need to modify this patch, as you suggested, until it 
>> becomes
>handy to be used reliably. We are on it, and will send another patch soon.
>>
>> But if somehow we are able to get some consistent results from live 
>> status, do
>you think it would be worth accepting this change, so that it can 
>handle soft HPDs and automation testing.
>> Because I believe we will face this problem whenever we are trying to 
>> test
>something from automation, where the physical device is not removed, 
>and DDC channel is up always.
>
>It's very well possible that all the platforms you have, but experience 
>says that some OEM will horrible screw this up. At least they've 
>consistently botched this in the past on occasional machines.
>
>Now the ghost hdmi detection on slow removal is obviously not great, 
>but we can't use the hpd bits to fix this. One approach would be.
>1. Upon hpd interrupt do an immediate probe of the connector. This way 
>we'll have good userspace experience if the unplug happens quickly and the hw works.
>2. Re-probe with a 1s delay to catch slow-uplugs. The current output 
>probing helpers are clever enough already that if a state-change 
>happens to be detected a uevent will be generate, irrespective of the 
>source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).
>
>Note that we already track the hpd interrupts on a per-source basis, so 
>doing the re-poll shouldn't be costly. Maybe do the re-poll as part of 
>the EDID invalidation to avoid stalling userspace.
>
>But you can't rely upon the hpd pins unfortunately :(
>
>This way we should be able to implement the 2 features you want, even 
>on unreliable hw.
>
>Cheers, Daniel
>--
>Daniel Vetter
>Software Engineer, Intel Corporation
>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
>_______________________________________________
>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

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Daniel Vetter]

Please don't drop the mailing list when discussing patches.

And nope, conditioning this on gen6+ won't help at all, since I have a
gen6 and gen7 machine here which don't have reliable hdmi live status.
Afaik that is _really_ broken across the board and going with the
fancy invalidation scheme and delayed work items is the way forward.

Cheers, Daniel

On Fri, Apr 11, 2014 at 2:51 PM, Sharma, Shashank
<shashank.sharma@intel.com> wrote:
> Hi Daniel,
>
> First of all, sorry for the delay in coming up with the new patch set. I got stuck with some commercial projects :) .
>
> There were few review comments what you gave for the previous optimization patch, those were:
> 1.  This might break the old HWs. Few of them might not even be HPD capable, so its required to handle them.
> 2.  Do not rely on the live_status check, as that HW is not yet proven.
> And you recommended to have a WQ, which will invalidate the cached HDMI EDID after a timeout.
>
> I have written another patch, which is addressing both of the above comments, but doesn't use a WQ.
>
> Before sending this to ML, I wanted to have an opinion from you, if this qualifies the design you were expecting.
> I will be really glad, if you can spare some time, and just have a top level view on the patch, and give your opinion.
>
> If you think that this is still not the way, and making a WQ is a better approach, I would gladly create a patch which applies that.
> Or if you will prefer to have this reviewed in the mailing list only, please accept my apologies for direct contact, and just let me know so. I will publish this in ML.
>
> The patch is (It's also attached):
>
> From e7ec4e4827615c0ff3a4ddb020df35fdf1b0b82f Mon Sep 17 00:00:00 2001
> From: Shashank Sharma <shashank.sharma@intel.com>
> Date: Fri, 11 Apr 2014 18:02:50 +0530
> Subject: [PATCH] drm/i915: Cache HDMI EDID for future reference
>
> This patch contains following changes:
> 1.Cache HDMI EDID and optimize detect() calls, which are
>   frequently called from userspace, causing un-necessary
>   EDID reads.
> 2.This cached EDID will be used for detection of the status of
>   HDMI connector, for Non HPD detect() calls. HPD calls will update
>   cached EDID.
> 3.This optimization is kept for new HW's (Gen6 and +), so that It
>   will not break old HWs who may not be even HPD capable.
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h  |    1 +
>  drivers/gpu/drm/i915/intel_hdmi.c |   28 ++++++++++++++++++++++++++--
>  2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 42762b7..b7911df 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -478,6 +478,7 @@ struct intel_hdmi {
>                                 const void *frame, ssize_t len);
>         void (*set_infoframes)(struct drm_encoder *encoder,
>                                struct drm_display_mode *adjusted_mode);
> +       struct edid *edid;
>  };
>
>  #define DP_MAX_DOWNSTREAM_PORTS                0x10
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index b0413e1..33550ca 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -938,7 +938,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>                 hdmi_to_dig_port(intel_hdmi);
>         struct intel_encoder *intel_encoder = &intel_dig_port->base;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct edid *edid;
> +       struct edid *edid = NULL;
>         enum intel_display_power_domain power_domain;
>         enum drm_connector_status status = connector_status_disconnected;
>
> @@ -948,6 +948,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>         power_domain = intel_display_port_power_domain(intel_encoder);
>         intel_display_power_get(dev_priv, power_domain);
>
> +       /*
> +       * Support EDID caching only for new architectures
> +       * Let old architectures probe and force read EDID
> +       */
> +       if (INTEL_INFO(dev)->gen >= 6) {
> +               if (force && intel_hdmi->edid &&
> +                       (connector->status != connector_status_unknown)) {
> +                       /* Optimize userspace query, read EDID only
> +                       in case of a real hot plug */
> +                       DRM_DEBUG_KMS("HDMI %s", intel_hdmi->edid ?
> +                               "Connected" : "Disconnected");
> +                       return connector->status;
> +               }
> +       }
> +
>         intel_hdmi->has_hdmi_sink = false;
>         intel_hdmi->has_audio = false;
>         intel_hdmi->rgb_quant_range_selectable = false;
> @@ -964,10 +979,19 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>                         intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
>                         intel_hdmi->rgb_quant_range_selectable =
>                                 drm_rgb_quant_range_selectable(edid);
> +                       DRM_DEBUG_KMS("HDMI Connected\n");
>                 }
> -               kfree(edid);
> +       } else {
> +               /*
> +               * Connector disconnected, free cached EDID
> +               * kfree is NULL protected, so will work for < gen 6 also */
> +               kfree(intel_hdmi->edid);
> +               DRM_DEBUG_KMS("HDMI Disconnected\n");
>         }
>
> +       /* Save latest EDID for further queries */
> +       intel_hdmi->edid = edid;
> +
>         if (status == connector_status_connected) {
>                 if (intel_hdmi->force_audio != HDMI_AUDIO_AUTO)
>                         intel_hdmi->has_audio =
> --
> 1.7.10.4
>
>
> -----Original Message-----
> From: Wang, Quanxian
> Sent: Thursday, April 10, 2014 4:12 PM
> To: Sharma, Shashank; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
>
> Tizen-IVI has one feature will require hdmi edid cache. It will be happy to find your patch pushed into upstream tree.
>
> Regards
>
> Thanks
>
> Quanxian Wang
>
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, Shashank
> Sent: Wednesday, April 09, 2014 12:20 PM
> To: Wang, Quanxian; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
>
> Hello Quanxian Wang
>
> This patch is available and working on all MCG tree's (Main, R42B and R44B) We were trying to opensource this patch, but due to the dependency on live_status reg, we had to change the design.
>
> I was working on that, but couldn't finish the activity yet, Thanks for reminding me I will update soon. :)
>
> Regards
> Shashank
> -----Original Message-----
> From: Wang, Quanxian
> Sent: Wednesday, April 09, 2014 11:49 AM
> To: Sharma, Shashank; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
>
> Hi, Sharma, Shashank
>
> Is there any following patches to make it happen?
>
> Thanks
>
> Regards
>
> Quanxian Wang
>
>>-----Original Message-----
>>From: intel-gfx-bounces@lists.freedesktop.org
>>[mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma,
>>Shashank
>>Sent: Tuesday, January 14, 2014 1:20 AM
>>To: Daniel Vetter
>>Cc: intel-gfx@lists.freedesktop.org
>>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect
>>and get_modes
>>
>>Thanks again for this explanation Daniel.
>>We will work on your suggestions and come up with a new patch.
>>
>>Regards
>>Shashank  / Ramalingam
>>-----Original Message-----
>>From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf
>>Of Daniel Vetter
>>Sent: Monday, January 13, 2014 6:57 PM
>>To: Sharma, Shashank
>>Cc: C, Ramalingam; intel-gfx@lists.freedesktop.org
>>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect
>>and get_modes
>>
>>On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank
>><shashank.sharma@intel.com> wrote:
>>> Thanks a lot for your time, for reviewing the changes, and giving us
>>> some
>>pointers.
>>> Both me and Ramalingam are designing this together, and we discussed
>>> about
>>these changes and your suggestions.
>>> There are few things we would like to discuss about. Please correct
>>> us if some of
>>our understanding is not proper.
>>
>>First something I've forgotten in the original mail: Overall your
>>patches look really nice and the commit messages and cover letter have been excellent.
>>Unfortunately you've run into one of the nastier cases of "reality just
>>wont agree with the spec" :(
>>
>>> Those two patches provide two solution.
>>> 1. Support for soft HPD, and slow removal of HDMI (when the DDC
>>> channel can
>>still get the EDID).
>>> 2. Try to reduce the EDID reads over DDC channel for get connector
>>> and fill mode
>>calls, by caching EDID, and using it until next HPD comes.
>>>
>>> Patch 2: Reduce the EDID read over DDC channel We are caching the
>>> EDID at every HPD up, on HDMI detect calls, and we are freeing it on
>>> subsequent
>>HDMI disconnect calls.
>>>
>>> The design philosophy here is, to maintain a state machine of HDMI
>>> connector
>>status, and differentiate between IOCTL detect calls and HPD detect calls.
>>> If there is a detect() or get_modes() call due to any of the IOCTL,
>>> which makes
>>sure that input variable force=1, we just use the cached EDID, to serve this calls.
>>> But if the detect call is coming from HPD work function, due to a HPD
>>> plug-out,
>>we remove/invalidate the old cached EDID, and cache the new EDID, on
>>subsequent HDMI plug-in.
>>> From here, the same state machine follows.
>>>
>>> Can you please let us know, why do you think that we should
>>> invalidate the
>>cached EDID after 1-2 seconds ?
>>
>>Because there are machines out there where hpd never happens. So if you
>>keep onto the cached value forever userspace will never notice a change
>>in output configuration. Of course hotplug handling won't work, but at
>>least users can still manually probe outputs. By unconditionally using
>>the cached edid from ioctls you break this use case. Yes, such machines
>>are broken, but we need to keep them working anyway.
>>
>>Also in my experience all machines are affected, we have examples
>>covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt yet
>>since we don't rely on the hpd bits any more (and so won't see bug reports any more).
>>
>>Generally if you use the hpd stuff your code must be designed under the
>>assumption that hpd is completely unreliably. We've seen anything from
>>random noise, flat-out not-working at all, stuck bits and unstable hpd
>>values that occasionally flip-flop. So you can't rely on it at all.
>>
>>> Note: In this same patch, there is additional optimization, which you
>>> pointed out,
>>where we check if the connector->status is same as live status.
>>> This can be removed independently, as you suggested.
>>
>>Hm, where have I pointed this out? Some other mail on internal discussions?
>>
>>> About patch 1:
>>> We have done some local experiments and we came to know that for VLV
>>> and
>>HSW boards, we can rely on the live status, if we give it some time to
>>settle (~300ms).
>>> Probably, we need to modify this patch, as you suggested, until it
>>> becomes
>>handy to be used reliably. We are on it, and will send another patch soon.
>>>
>>> But if somehow we are able to get some consistent results from live
>>> status, do
>>you think it would be worth accepting this change, so that it can
>>handle soft HPDs and automation testing.
>>> Because I believe we will face this problem whenever we are trying to
>>> test
>>something from automation, where the physical device is not removed,
>>and DDC channel is up always.
>>
>>It's very well possible that all the platforms you have, but experience
>>says that some OEM will horrible screw this up. At least they've
>>consistently botched this in the past on occasional machines.
>>
>>Now the ghost hdmi detection on slow removal is obviously not great,
>>but we can't use the hpd bits to fix this. One approach would be.
>>1. Upon hpd interrupt do an immediate probe of the connector. This way
>>we'll have good userspace experience if the unplug happens quickly and the hw works.
>>2. Re-probe with a 1s delay to catch slow-uplugs. The current output
>>probing helpers are clever enough already that if a state-change
>>happens to be detected a uevent will be generate, irrespective of the
>>source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).
>>
>>Note that we already track the hpd interrupts on a per-source basis, so
>>doing the re-poll shouldn't be costly. Maybe do the re-poll as part of
>>the EDID invalidation to avoid stalling userspace.
>>
>>But you can't rely upon the hpd pins unfortunately :(
>>
>>This way we should be able to implement the 2 features you want, even
>>on unreliable hw.
>>
>>Cheers, Daniel
>>--
>>Daniel Vetter
>>Software Engineer, Intel Corporation
>>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>_______________________________________________
>>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
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Sharma, Shashank]

Thanks for the comments, 
Actually, we are not using live_status at all. 

The check for < gen6 is only for EDID caching. So if the HW is >= gen6 cache_edid.  
Else do not cache EDID, so that we will not block any of the old HW, which might not be HPD capable.

Does it sound ok now :) ?  

Regards
Shashank 
-----Original Message-----
From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Friday, April 11, 2014 6:28 PM
To: Sharma, Shashank
Cc: C, Ramalingam; Wang, Quanxian; intel-gfx
Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

Please don't drop the mailing list when discussing patches.

And nope, conditioning this on gen6+ won't help at all, since I have a
gen6 and gen7 machine here which don't have reliable hdmi live status.
Afaik that is _really_ broken across the board and going with the fancy invalidation scheme and delayed work items is the way forward.

Cheers, Daniel

On Fri, Apr 11, 2014 at 2:51 PM, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> Hi Daniel,
>
> First of all, sorry for the delay in coming up with the new patch set. I got stuck with some commercial projects :) .
>
> There were few review comments what you gave for the previous optimization patch, those were:
> 1.  This might break the old HWs. Few of them might not even be HPD capable, so its required to handle them.
> 2.  Do not rely on the live_status check, as that HW is not yet proven.
> And you recommended to have a WQ, which will invalidate the cached HDMI EDID after a timeout.
>
> I have written another patch, which is addressing both of the above comments, but doesn't use a WQ.
>
> Before sending this to ML, I wanted to have an opinion from you, if this qualifies the design you were expecting.
> I will be really glad, if you can spare some time, and just have a top level view on the patch, and give your opinion.
>
> If you think that this is still not the way, and making a WQ is a better approach, I would gladly create a patch which applies that.
> Or if you will prefer to have this reviewed in the mailing list only, please accept my apologies for direct contact, and just let me know so. I will publish this in ML.
>
> The patch is (It's also attached):
>
> From e7ec4e4827615c0ff3a4ddb020df35fdf1b0b82f Mon Sep 17 00:00:00 2001
> From: Shashank Sharma <shashank.sharma@intel.com>
> Date: Fri, 11 Apr 2014 18:02:50 +0530
> Subject: [PATCH] drm/i915: Cache HDMI EDID for future reference
>
> This patch contains following changes:
> 1.Cache HDMI EDID and optimize detect() calls, which are
>   frequently called from userspace, causing un-necessary
>   EDID reads.
> 2.This cached EDID will be used for detection of the status of
>   HDMI connector, for Non HPD detect() calls. HPD calls will update
>   cached EDID.
> 3.This optimization is kept for new HW's (Gen6 and +), so that It
>   will not break old HWs who may not be even HPD capable.
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h  |    1 +
>  drivers/gpu/drm/i915/intel_hdmi.c |   28 ++++++++++++++++++++++++++--
>  2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 42762b7..b7911df 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -478,6 +478,7 @@ struct intel_hdmi {
>                                 const void *frame, ssize_t len);
>         void (*set_infoframes)(struct drm_encoder *encoder,
>                                struct drm_display_mode 
> *adjusted_mode);
> +       struct edid *edid;
>  };
>
>  #define DP_MAX_DOWNSTREAM_PORTS                0x10
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index b0413e1..33550ca 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -938,7 +938,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>                 hdmi_to_dig_port(intel_hdmi);
>         struct intel_encoder *intel_encoder = &intel_dig_port->base;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct edid *edid;
> +       struct edid *edid = NULL;
>         enum intel_display_power_domain power_domain;
>         enum drm_connector_status status = 
> connector_status_disconnected;
>
> @@ -948,6 +948,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>         power_domain = intel_display_port_power_domain(intel_encoder);
>         intel_display_power_get(dev_priv, power_domain);
>
> +       /*
> +       * Support EDID caching only for new architectures
> +       * Let old architectures probe and force read EDID
> +       */
> +       if (INTEL_INFO(dev)->gen >= 6) {
> +               if (force && intel_hdmi->edid &&
> +                       (connector->status != connector_status_unknown)) {
> +                       /* Optimize userspace query, read EDID only
> +                       in case of a real hot plug */
> +                       DRM_DEBUG_KMS("HDMI %s", intel_hdmi->edid ?
> +                               "Connected" : "Disconnected");
> +                       return connector->status;
> +               }
> +       }
> +
>         intel_hdmi->has_hdmi_sink = false;
>         intel_hdmi->has_audio = false;
>         intel_hdmi->rgb_quant_range_selectable = false; @@ -964,10 
> +979,19 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>                         intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
>                         intel_hdmi->rgb_quant_range_selectable =
>                                 drm_rgb_quant_range_selectable(edid);
> +                       DRM_DEBUG_KMS("HDMI Connected\n");
>                 }
> -               kfree(edid);
> +       } else {
> +               /*
> +               * Connector disconnected, free cached EDID
> +               * kfree is NULL protected, so will work for < gen 6 also */
> +               kfree(intel_hdmi->edid);
> +               DRM_DEBUG_KMS("HDMI Disconnected\n");
>         }
>
> +       /* Save latest EDID for further queries */
> +       intel_hdmi->edid = edid;
> +
>         if (status == connector_status_connected) {
>                 if (intel_hdmi->force_audio != HDMI_AUDIO_AUTO)
>                         intel_hdmi->has_audio =
> --
> 1.7.10.4
>
>
> -----Original Message-----
> From: Wang, Quanxian
> Sent: Thursday, April 10, 2014 4:12 PM
> To: Sharma, Shashank; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> and get_modes
>
> Tizen-IVI has one feature will require hdmi edid cache. It will be happy to find your patch pushed into upstream tree.
>
> Regards
>
> Thanks
>
> Quanxian Wang
>
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
> Behalf Of Sharma, Shashank
> Sent: Wednesday, April 09, 2014 12:20 PM
> To: Wang, Quanxian; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> and get_modes
>
> Hello Quanxian Wang
>
> This patch is available and working on all MCG tree's (Main, R42B and R44B) We were trying to opensource this patch, but due to the dependency on live_status reg, we had to change the design.
>
> I was working on that, but couldn't finish the activity yet, Thanks 
> for reminding me I will update soon. :)
>
> Regards
> Shashank
> -----Original Message-----
> From: Wang, Quanxian
> Sent: Wednesday, April 09, 2014 11:49 AM
> To: Sharma, Shashank; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> and get_modes
>
> Hi, Sharma, Shashank
>
> Is there any following patches to make it happen?
>
> Thanks
>
> Regards
>
> Quanxian Wang
>
>>-----Original Message-----
>>From: intel-gfx-bounces@lists.freedesktop.org
>>[mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, 
>>Shashank
>>Sent: Tuesday, January 14, 2014 1:20 AM
>>To: Daniel Vetter
>>Cc: intel-gfx@lists.freedesktop.org
>>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
>>and get_modes
>>
>>Thanks again for this explanation Daniel.
>>We will work on your suggestions and come up with a new patch.
>>
>>Regards
>>Shashank  / Ramalingam
>>-----Original Message-----
>>From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf 
>>Of Daniel Vetter
>>Sent: Monday, January 13, 2014 6:57 PM
>>To: Sharma, Shashank
>>Cc: C, Ramalingam; intel-gfx@lists.freedesktop.org
>>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
>>and get_modes
>>
>>On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank 
>><shashank.sharma@intel.com> wrote:
>>> Thanks a lot for your time, for reviewing the changes, and giving us 
>>> some
>>pointers.
>>> Both me and Ramalingam are designing this together, and we discussed 
>>> about
>>these changes and your suggestions.
>>> There are few things we would like to discuss about. Please correct 
>>> us if some of
>>our understanding is not proper.
>>
>>First something I've forgotten in the original mail: Overall your 
>>patches look really nice and the commit messages and cover letter have been excellent.
>>Unfortunately you've run into one of the nastier cases of "reality 
>>just wont agree with the spec" :(
>>
>>> Those two patches provide two solution.
>>> 1. Support for soft HPD, and slow removal of HDMI (when the DDC 
>>> channel can
>>still get the EDID).
>>> 2. Try to reduce the EDID reads over DDC channel for get connector 
>>> and fill mode
>>calls, by caching EDID, and using it until next HPD comes.
>>>
>>> Patch 2: Reduce the EDID read over DDC channel We are caching the 
>>> EDID at every HPD up, on HDMI detect calls, and we are freeing it on 
>>> subsequent
>>HDMI disconnect calls.
>>>
>>> The design philosophy here is, to maintain a state machine of HDMI 
>>> connector
>>status, and differentiate between IOCTL detect calls and HPD detect calls.
>>> If there is a detect() or get_modes() call due to any of the IOCTL, 
>>> which makes
>>sure that input variable force=1, we just use the cached EDID, to serve this calls.
>>> But if the detect call is coming from HPD work function, due to a 
>>> HPD plug-out,
>>we remove/invalidate the old cached EDID, and cache the new EDID, on 
>>subsequent HDMI plug-in.
>>> From here, the same state machine follows.
>>>
>>> Can you please let us know, why do you think that we should 
>>> invalidate the
>>cached EDID after 1-2 seconds ?
>>
>>Because there are machines out there where hpd never happens. So if 
>>you keep onto the cached value forever userspace will never notice a 
>>change in output configuration. Of course hotplug handling won't work, 
>>but at least users can still manually probe outputs. By 
>>unconditionally using the cached edid from ioctls you break this use 
>>case. Yes, such machines are broken, but we need to keep them working anyway.
>>
>>Also in my experience all machines are affected, we have examples 
>>covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt yet 
>>since we don't rely on the hpd bits any more (and so won't see bug reports any more).
>>
>>Generally if you use the hpd stuff your code must be designed under 
>>the assumption that hpd is completely unreliably. We've seen anything 
>>from random noise, flat-out not-working at all, stuck bits and 
>>unstable hpd values that occasionally flip-flop. So you can't rely on it at all.
>>
>>> Note: In this same patch, there is additional optimization, which 
>>> you pointed out,
>>where we check if the connector->status is same as live status.
>>> This can be removed independently, as you suggested.
>>
>>Hm, where have I pointed this out? Some other mail on internal discussions?
>>
>>> About patch 1:
>>> We have done some local experiments and we came to know that for VLV 
>>> and
>>HSW boards, we can rely on the live status, if we give it some time to 
>>settle (~300ms).
>>> Probably, we need to modify this patch, as you suggested, until it 
>>> becomes
>>handy to be used reliably. We are on it, and will send another patch soon.
>>>
>>> But if somehow we are able to get some consistent results from live 
>>> status, do
>>you think it would be worth accepting this change, so that it can 
>>handle soft HPDs and automation testing.
>>> Because I believe we will face this problem whenever we are trying 
>>> to test
>>something from automation, where the physical device is not removed, 
>>and DDC channel is up always.
>>
>>It's very well possible that all the platforms you have, but 
>>experience says that some OEM will horrible screw this up. At least 
>>they've consistently botched this in the past on occasional machines.
>>
>>Now the ghost hdmi detection on slow removal is obviously not great, 
>>but we can't use the hpd bits to fix this. One approach would be.
>>1. Upon hpd interrupt do an immediate probe of the connector. This way 
>>we'll have good userspace experience if the unplug happens quickly and the hw works.
>>2. Re-probe with a 1s delay to catch slow-uplugs. The current output 
>>probing helpers are clever enough already that if a state-change 
>>happens to be detected a uevent will be generate, irrespective of the 
>>source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).
>>
>>Note that we already track the hpd interrupts on a per-source basis, 
>>so doing the re-poll shouldn't be costly. Maybe do the re-poll as part 
>>of the EDID invalidation to avoid stalling userspace.
>>
>>But you can't rely upon the hpd pins unfortunately :(
>>
>>This way we should be able to implement the 2 features you want, even 
>>on unreliable hw.
>>
>>Cheers, Daniel
>>--
>>Daniel Vetter
>>Software Engineer, Intel Corporation
>>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>_______________________________________________
>>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
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Daniel Vetter]

On Fri, Apr 11, 2014 at 01:23:43PM +0000, Sharma, Shashank wrote:
> Thanks for the comments, 
> Actually, we are not using live_status at all. 
> 
> The check for < gen6 is only for EDID caching. So if the HW is >= gen6 cache_edid.  
> Else do not cache EDID, so that we will not block any of the old HW, which might not be HPD capable.

Oh, I've thought that this is incremental on top of something you already
have.
> 
> Does it sound ok now :) ?  

No. HPD is _NOT_ I repeat _NOT_ reliably. Not even on gen6+. live status
simply reflects the hpd pin, if either doesn't work, then neither does the
other one.

Nacked-with-prejudice-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Daniel

> 
> Regards
> Shashank 
> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Friday, April 11, 2014 6:28 PM
> To: Sharma, Shashank
> Cc: C, Ramalingam; Wang, Quanxian; intel-gfx
> Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
> 
> Please don't drop the mailing list when discussing patches.
> 
> And nope, conditioning this on gen6+ won't help at all, since I have a
> gen6 and gen7 machine here which don't have reliable hdmi live status.
> Afaik that is _really_ broken across the board and going with the fancy invalidation scheme and delayed work items is the way forward.
> 
> Cheers, Daniel
> 
> On Fri, Apr 11, 2014 at 2:51 PM, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> > Hi Daniel,
> >
> > First of all, sorry for the delay in coming up with the new patch set. I got stuck with some commercial projects :) .
> >
> > There were few review comments what you gave for the previous optimization patch, those were:
> > 1.  This might break the old HWs. Few of them might not even be HPD capable, so its required to handle them.
> > 2.  Do not rely on the live_status check, as that HW is not yet proven.
> > And you recommended to have a WQ, which will invalidate the cached HDMI EDID after a timeout.
> >
> > I have written another patch, which is addressing both of the above comments, but doesn't use a WQ.
> >
> > Before sending this to ML, I wanted to have an opinion from you, if this qualifies the design you were expecting.
> > I will be really glad, if you can spare some time, and just have a top level view on the patch, and give your opinion.
> >
> > If you think that this is still not the way, and making a WQ is a better approach, I would gladly create a patch which applies that.
> > Or if you will prefer to have this reviewed in the mailing list only, please accept my apologies for direct contact, and just let me know so. I will publish this in ML.
> >
> > The patch is (It's also attached):
> >
> > From e7ec4e4827615c0ff3a4ddb020df35fdf1b0b82f Mon Sep 17 00:00:00 2001
> > From: Shashank Sharma <shashank.sharma@intel.com>
> > Date: Fri, 11 Apr 2014 18:02:50 +0530
> > Subject: [PATCH] drm/i915: Cache HDMI EDID for future reference
> >
> > This patch contains following changes:
> > 1.Cache HDMI EDID and optimize detect() calls, which are
> >   frequently called from userspace, causing un-necessary
> >   EDID reads.
> > 2.This cached EDID will be used for detection of the status of
> >   HDMI connector, for Non HPD detect() calls. HPD calls will update
> >   cached EDID.
> > 3.This optimization is kept for new HW's (Gen6 and +), so that It
> >   will not break old HWs who may not be even HPD capable.
> >
> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h  |    1 +
> >  drivers/gpu/drm/i915/intel_hdmi.c |   28 ++++++++++++++++++++++++++--
> >  2 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 42762b7..b7911df 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -478,6 +478,7 @@ struct intel_hdmi {
> >                                 const void *frame, ssize_t len);
> >         void (*set_infoframes)(struct drm_encoder *encoder,
> >                                struct drm_display_mode 
> > *adjusted_mode);
> > +       struct edid *edid;
> >  };
> >
> >  #define DP_MAX_DOWNSTREAM_PORTS                0x10
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index b0413e1..33550ca 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -938,7 +938,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> >                 hdmi_to_dig_port(intel_hdmi);
> >         struct intel_encoder *intel_encoder = &intel_dig_port->base;
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct edid *edid;
> > +       struct edid *edid = NULL;
> >         enum intel_display_power_domain power_domain;
> >         enum drm_connector_status status = 
> > connector_status_disconnected;
> >
> > @@ -948,6 +948,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> >         power_domain = intel_display_port_power_domain(intel_encoder);
> >         intel_display_power_get(dev_priv, power_domain);
> >
> > +       /*
> > +       * Support EDID caching only for new architectures
> > +       * Let old architectures probe and force read EDID
> > +       */
> > +       if (INTEL_INFO(dev)->gen >= 6) {
> > +               if (force && intel_hdmi->edid &&
> > +                       (connector->status != connector_status_unknown)) {
> > +                       /* Optimize userspace query, read EDID only
> > +                       in case of a real hot plug */
> > +                       DRM_DEBUG_KMS("HDMI %s", intel_hdmi->edid ?
> > +                               "Connected" : "Disconnected");
> > +                       return connector->status;
> > +               }
> > +       }
> > +
> >         intel_hdmi->has_hdmi_sink = false;
> >         intel_hdmi->has_audio = false;
> >         intel_hdmi->rgb_quant_range_selectable = false; @@ -964,10 
> > +979,19 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> >                         intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> >                         intel_hdmi->rgb_quant_range_selectable =
> >                                 drm_rgb_quant_range_selectable(edid);
> > +                       DRM_DEBUG_KMS("HDMI Connected\n");
> >                 }
> > -               kfree(edid);
> > +       } else {
> > +               /*
> > +               * Connector disconnected, free cached EDID
> > +               * kfree is NULL protected, so will work for < gen 6 also */
> > +               kfree(intel_hdmi->edid);
> > +               DRM_DEBUG_KMS("HDMI Disconnected\n");
> >         }
> >
> > +       /* Save latest EDID for further queries */
> > +       intel_hdmi->edid = edid;
> > +
> >         if (status == connector_status_connected) {
> >                 if (intel_hdmi->force_audio != HDMI_AUDIO_AUTO)
> >                         intel_hdmi->has_audio =
> > --
> > 1.7.10.4
> >
> >
> > -----Original Message-----
> > From: Wang, Quanxian
> > Sent: Thursday, April 10, 2014 4:12 PM
> > To: Sharma, Shashank; Daniel Vetter
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> > and get_modes
> >
> > Tizen-IVI has one feature will require hdmi edid cache. It will be happy to find your patch pushed into upstream tree.
> >
> > Regards
> >
> > Thanks
> >
> > Quanxian Wang
> >
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
> > Behalf Of Sharma, Shashank
> > Sent: Wednesday, April 09, 2014 12:20 PM
> > To: Wang, Quanxian; Daniel Vetter
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> > and get_modes
> >
> > Hello Quanxian Wang
> >
> > This patch is available and working on all MCG tree's (Main, R42B and R44B) We were trying to opensource this patch, but due to the dependency on live_status reg, we had to change the design.
> >
> > I was working on that, but couldn't finish the activity yet, Thanks 
> > for reminding me I will update soon. :)
> >
> > Regards
> > Shashank
> > -----Original Message-----
> > From: Wang, Quanxian
> > Sent: Wednesday, April 09, 2014 11:49 AM
> > To: Sharma, Shashank; Daniel Vetter
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> > and get_modes
> >
> > Hi, Sharma, Shashank
> >
> > Is there any following patches to make it happen?
> >
> > Thanks
> >
> > Regards
> >
> > Quanxian Wang
> >
> >>-----Original Message-----
> >>From: intel-gfx-bounces@lists.freedesktop.org
> >>[mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, 
> >>Shashank
> >>Sent: Tuesday, January 14, 2014 1:20 AM
> >>To: Daniel Vetter
> >>Cc: intel-gfx@lists.freedesktop.org
> >>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> >>and get_modes
> >>
> >>Thanks again for this explanation Daniel.
> >>We will work on your suggestions and come up with a new patch.
> >>
> >>Regards
> >>Shashank  / Ramalingam
> >>-----Original Message-----
> >>From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf 
> >>Of Daniel Vetter
> >>Sent: Monday, January 13, 2014 6:57 PM
> >>To: Sharma, Shashank
> >>Cc: C, Ramalingam; intel-gfx@lists.freedesktop.org
> >>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> >>and get_modes
> >>
> >>On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank 
> >><shashank.sharma@intel.com> wrote:
> >>> Thanks a lot for your time, for reviewing the changes, and giving us 
> >>> some
> >>pointers.
> >>> Both me and Ramalingam are designing this together, and we discussed 
> >>> about
> >>these changes and your suggestions.
> >>> There are few things we would like to discuss about. Please correct 
> >>> us if some of
> >>our understanding is not proper.
> >>
> >>First something I've forgotten in the original mail: Overall your 
> >>patches look really nice and the commit messages and cover letter have been excellent.
> >>Unfortunately you've run into one of the nastier cases of "reality 
> >>just wont agree with the spec" :(
> >>
> >>> Those two patches provide two solution.
> >>> 1. Support for soft HPD, and slow removal of HDMI (when the DDC 
> >>> channel can
> >>still get the EDID).
> >>> 2. Try to reduce the EDID reads over DDC channel for get connector 
> >>> and fill mode
> >>calls, by caching EDID, and using it until next HPD comes.
> >>>
> >>> Patch 2: Reduce the EDID read over DDC channel We are caching the 
> >>> EDID at every HPD up, on HDMI detect calls, and we are freeing it on 
> >>> subsequent
> >>HDMI disconnect calls.
> >>>
> >>> The design philosophy here is, to maintain a state machine of HDMI 
> >>> connector
> >>status, and differentiate between IOCTL detect calls and HPD detect calls.
> >>> If there is a detect() or get_modes() call due to any of the IOCTL, 
> >>> which makes
> >>sure that input variable force=1, we just use the cached EDID, to serve this calls.
> >>> But if the detect call is coming from HPD work function, due to a 
> >>> HPD plug-out,
> >>we remove/invalidate the old cached EDID, and cache the new EDID, on 
> >>subsequent HDMI plug-in.
> >>> From here, the same state machine follows.
> >>>
> >>> Can you please let us know, why do you think that we should 
> >>> invalidate the
> >>cached EDID after 1-2 seconds ?
> >>
> >>Because there are machines out there where hpd never happens. So if 
> >>you keep onto the cached value forever userspace will never notice a 
> >>change in output configuration. Of course hotplug handling won't work, 
> >>but at least users can still manually probe outputs. By 
> >>unconditionally using the cached edid from ioctls you break this use 
> >>case. Yes, such machines are broken, but we need to keep them working anyway.
> >>
> >>Also in my experience all machines are affected, we have examples 
> >>covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt yet 
> >>since we don't rely on the hpd bits any more (and so won't see bug reports any more).
> >>
> >>Generally if you use the hpd stuff your code must be designed under 
> >>the assumption that hpd is completely unreliably. We've seen anything 
> >>from random noise, flat-out not-working at all, stuck bits and 
> >>unstable hpd values that occasionally flip-flop. So you can't rely on it at all.
> >>
> >>> Note: In this same patch, there is additional optimization, which 
> >>> you pointed out,
> >>where we check if the connector->status is same as live status.
> >>> This can be removed independently, as you suggested.
> >>
> >>Hm, where have I pointed this out? Some other mail on internal discussions?
> >>
> >>> About patch 1:
> >>> We have done some local experiments and we came to know that for VLV 
> >>> and
> >>HSW boards, we can rely on the live status, if we give it some time to 
> >>settle (~300ms).
> >>> Probably, we need to modify this patch, as you suggested, until it 
> >>> becomes
> >>handy to be used reliably. We are on it, and will send another patch soon.
> >>>
> >>> But if somehow we are able to get some consistent results from live 
> >>> status, do
> >>you think it would be worth accepting this change, so that it can 
> >>handle soft HPDs and automation testing.
> >>> Because I believe we will face this problem whenever we are trying 
> >>> to test
> >>something from automation, where the physical device is not removed, 
> >>and DDC channel is up always.
> >>
> >>It's very well possible that all the platforms you have, but 
> >>experience says that some OEM will horrible screw this up. At least 
> >>they've consistently botched this in the past on occasional machines.
> >>
> >>Now the ghost hdmi detection on slow removal is obviously not great, 
> >>but we can't use the hpd bits to fix this. One approach would be.
> >>1. Upon hpd interrupt do an immediate probe of the connector. This way 
> >>we'll have good userspace experience if the unplug happens quickly and the hw works.
> >>2. Re-probe with a 1s delay to catch slow-uplugs. The current output 
> >>probing helpers are clever enough already that if a state-change 
> >>happens to be detected a uevent will be generate, irrespective of the 
> >>source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).
> >>
> >>Note that we already track the hpd interrupts on a per-source basis, 
> >>so doing the re-poll shouldn't be costly. Maybe do the re-poll as part 
> >>of the EDID invalidation to avoid stalling userspace.
> >>
> >>But you can't rely upon the hpd pins unfortunately :(
> >>
> >>This way we should be able to implement the 2 features you want, even 
> >>on unreliable hw.
> >>
> >>Cheers, Daniel
> >>--
> >>Daniel Vetter
> >>Software Engineer, Intel Corporation
> >>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >>_______________________________________________
> >>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
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Sharma, Shashank]

Ok, I will change the implementation. 

Regards
Shashank 
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Friday, April 11, 2014 7:53 PM
To: Sharma, Shashank
Cc: Daniel Vetter; C, Ramalingam; Wang, Quanxian; intel-gfx
Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

On Fri, Apr 11, 2014 at 01:23:43PM +0000, Sharma, Shashank wrote:
> Thanks for the comments,
> Actually, we are not using live_status at all. 
> 
> The check for < gen6 is only for EDID caching. So if the HW is >= gen6 cache_edid.  
> Else do not cache EDID, so that we will not block any of the old HW, which might not be HPD capable.

Oh, I've thought that this is incremental on top of something you already have.
> 
> Does it sound ok now :) ?  

No. HPD is _NOT_ I repeat _NOT_ reliably. Not even on gen6+. live status simply reflects the hpd pin, if either doesn't work, then neither does the other one.

Nacked-with-prejudice-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Daniel

> 
> Regards
> Shashank
> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf 
> Of Daniel Vetter
> Sent: Friday, April 11, 2014 6:28 PM
> To: Sharma, Shashank
> Cc: C, Ramalingam; Wang, Quanxian; intel-gfx
> Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> and get_modes
> 
> Please don't drop the mailing list when discussing patches.
> 
> And nope, conditioning this on gen6+ won't help at all, since I have a
> gen6 and gen7 machine here which don't have reliable hdmi live status.
> Afaik that is _really_ broken across the board and going with the fancy invalidation scheme and delayed work items is the way forward.
> 
> Cheers, Daniel
> 
> On Fri, Apr 11, 2014 at 2:51 PM, Sharma, Shashank <shashank.sharma@intel.com> wrote:
> > Hi Daniel,
> >
> > First of all, sorry for the delay in coming up with the new patch set. I got stuck with some commercial projects :) .
> >
> > There were few review comments what you gave for the previous optimization patch, those were:
> > 1.  This might break the old HWs. Few of them might not even be HPD capable, so its required to handle them.
> > 2.  Do not rely on the live_status check, as that HW is not yet proven.
> > And you recommended to have a WQ, which will invalidate the cached HDMI EDID after a timeout.
> >
> > I have written another patch, which is addressing both of the above comments, but doesn't use a WQ.
> >
> > Before sending this to ML, I wanted to have an opinion from you, if this qualifies the design you were expecting.
> > I will be really glad, if you can spare some time, and just have a top level view on the patch, and give your opinion.
> >
> > If you think that this is still not the way, and making a WQ is a better approach, I would gladly create a patch which applies that.
> > Or if you will prefer to have this reviewed in the mailing list only, please accept my apologies for direct contact, and just let me know so. I will publish this in ML.
> >
> > The patch is (It's also attached):
> >
> > From e7ec4e4827615c0ff3a4ddb020df35fdf1b0b82f Mon Sep 17 00:00:00 
> > 2001
> > From: Shashank Sharma <shashank.sharma@intel.com>
> > Date: Fri, 11 Apr 2014 18:02:50 +0530
> > Subject: [PATCH] drm/i915: Cache HDMI EDID for future reference
> >
> > This patch contains following changes:
> > 1.Cache HDMI EDID and optimize detect() calls, which are
> >   frequently called from userspace, causing un-necessary
> >   EDID reads.
> > 2.This cached EDID will be used for detection of the status of
> >   HDMI connector, for Non HPD detect() calls. HPD calls will update
> >   cached EDID.
> > 3.This optimization is kept for new HW's (Gen6 and +), so that It
> >   will not break old HWs who may not be even HPD capable.
> >
> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h  |    1 +
> >  drivers/gpu/drm/i915/intel_hdmi.c |   28 ++++++++++++++++++++++++++--
> >  2 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 42762b7..b7911df 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -478,6 +478,7 @@ struct intel_hdmi {
> >                                 const void *frame, ssize_t len);
> >         void (*set_infoframes)(struct drm_encoder *encoder,
> >                                struct drm_display_mode 
> > *adjusted_mode);
> > +       struct edid *edid;
> >  };
> >
> >  #define DP_MAX_DOWNSTREAM_PORTS                0x10
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index b0413e1..33550ca 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -938,7 +938,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> >                 hdmi_to_dig_port(intel_hdmi);
> >         struct intel_encoder *intel_encoder = &intel_dig_port->base;
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct edid *edid;
> > +       struct edid *edid = NULL;
> >         enum intel_display_power_domain power_domain;
> >         enum drm_connector_status status = 
> > connector_status_disconnected;
> >
> > @@ -948,6 +948,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> >         power_domain = intel_display_port_power_domain(intel_encoder);
> >         intel_display_power_get(dev_priv, power_domain);
> >
> > +       /*
> > +       * Support EDID caching only for new architectures
> > +       * Let old architectures probe and force read EDID
> > +       */
> > +       if (INTEL_INFO(dev)->gen >= 6) {
> > +               if (force && intel_hdmi->edid &&
> > +                       (connector->status != connector_status_unknown)) {
> > +                       /* Optimize userspace query, read EDID only
> > +                       in case of a real hot plug */
> > +                       DRM_DEBUG_KMS("HDMI %s", intel_hdmi->edid ?
> > +                               "Connected" : "Disconnected");
> > +                       return connector->status;
> > +               }
> > +       }
> > +
> >         intel_hdmi->has_hdmi_sink = false;
> >         intel_hdmi->has_audio = false;
> >         intel_hdmi->rgb_quant_range_selectable = false; @@ -964,10
> > +979,19 @@ intel_hdmi_detect(struct drm_connector *connector, bool 
> > +force)
> >                         intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> >                         intel_hdmi->rgb_quant_range_selectable =
> >                                 
> > drm_rgb_quant_range_selectable(edid);
> > +                       DRM_DEBUG_KMS("HDMI Connected\n");
> >                 }
> > -               kfree(edid);
> > +       } else {
> > +               /*
> > +               * Connector disconnected, free cached EDID
> > +               * kfree is NULL protected, so will work for < gen 6 also */
> > +               kfree(intel_hdmi->edid);
> > +               DRM_DEBUG_KMS("HDMI Disconnected\n");
> >         }
> >
> > +       /* Save latest EDID for further queries */
> > +       intel_hdmi->edid = edid;
> > +
> >         if (status == connector_status_connected) {
> >                 if (intel_hdmi->force_audio != HDMI_AUDIO_AUTO)
> >                         intel_hdmi->has_audio =
> > --
> > 1.7.10.4
> >
> >
> > -----Original Message-----
> > From: Wang, Quanxian
> > Sent: Thursday, April 10, 2014 4:12 PM
> > To: Sharma, Shashank; Daniel Vetter
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI 
> > detect and get_modes
> >
> > Tizen-IVI has one feature will require hdmi edid cache. It will be happy to find your patch pushed into upstream tree.
> >
> > Regards
> >
> > Thanks
> >
> > Quanxian Wang
> >
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
> > Behalf Of Sharma, Shashank
> > Sent: Wednesday, April 09, 2014 12:20 PM
> > To: Wang, Quanxian; Daniel Vetter
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI 
> > detect and get_modes
> >
> > Hello Quanxian Wang
> >
> > This patch is available and working on all MCG tree's (Main, R42B and R44B) We were trying to opensource this patch, but due to the dependency on live_status reg, we had to change the design.
> >
> > I was working on that, but couldn't finish the activity yet, Thanks 
> > for reminding me I will update soon. :)
> >
> > Regards
> > Shashank
> > -----Original Message-----
> > From: Wang, Quanxian
> > Sent: Wednesday, April 09, 2014 11:49 AM
> > To: Sharma, Shashank; Daniel Vetter
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI 
> > detect and get_modes
> >
> > Hi, Sharma, Shashank
> >
> > Is there any following patches to make it happen?
> >
> > Thanks
> >
> > Regards
> >
> > Quanxian Wang
> >
> >>-----Original Message-----
> >>From: intel-gfx-bounces@lists.freedesktop.org
> >>[mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of 
> >>Sharma, Shashank
> >>Sent: Tuesday, January 14, 2014 1:20 AM
> >>To: Daniel Vetter
> >>Cc: intel-gfx@lists.freedesktop.org
> >>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI 
> >>detect and get_modes
> >>
> >>Thanks again for this explanation Daniel.
> >>We will work on your suggestions and come up with a new patch.
> >>
> >>Regards
> >>Shashank  / Ramalingam
> >>-----Original Message-----
> >>From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On 
> >>Behalf Of Daniel Vetter
> >>Sent: Monday, January 13, 2014 6:57 PM
> >>To: Sharma, Shashank
> >>Cc: C, Ramalingam; intel-gfx@lists.freedesktop.org
> >>Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI 
> >>detect and get_modes
> >>
> >>On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank 
> >><shashank.sharma@intel.com> wrote:
> >>> Thanks a lot for your time, for reviewing the changes, and giving 
> >>> us some
> >>pointers.
> >>> Both me and Ramalingam are designing this together, and we 
> >>> discussed about
> >>these changes and your suggestions.
> >>> There are few things we would like to discuss about. Please 
> >>> correct us if some of
> >>our understanding is not proper.
> >>
> >>First something I've forgotten in the original mail: Overall your 
> >>patches look really nice and the commit messages and cover letter have been excellent.
> >>Unfortunately you've run into one of the nastier cases of "reality 
> >>just wont agree with the spec" :(
> >>
> >>> Those two patches provide two solution.
> >>> 1. Support for soft HPD, and slow removal of HDMI (when the DDC 
> >>> channel can
> >>still get the EDID).
> >>> 2. Try to reduce the EDID reads over DDC channel for get connector 
> >>> and fill mode
> >>calls, by caching EDID, and using it until next HPD comes.
> >>>
> >>> Patch 2: Reduce the EDID read over DDC channel We are caching the 
> >>> EDID at every HPD up, on HDMI detect calls, and we are freeing it 
> >>> on subsequent
> >>HDMI disconnect calls.
> >>>
> >>> The design philosophy here is, to maintain a state machine of HDMI 
> >>> connector
> >>status, and differentiate between IOCTL detect calls and HPD detect calls.
> >>> If there is a detect() or get_modes() call due to any of the 
> >>> IOCTL, which makes
> >>sure that input variable force=1, we just use the cached EDID, to serve this calls.
> >>> But if the detect call is coming from HPD work function, due to a 
> >>> HPD plug-out,
> >>we remove/invalidate the old cached EDID, and cache the new EDID, on 
> >>subsequent HDMI plug-in.
> >>> From here, the same state machine follows.
> >>>
> >>> Can you please let us know, why do you think that we should 
> >>> invalidate the
> >>cached EDID after 1-2 seconds ?
> >>
> >>Because there are machines out there where hpd never happens. So if 
> >>you keep onto the cached value forever userspace will never notice a 
> >>change in output configuration. Of course hotplug handling won't 
> >>work, but at least users can still manually probe outputs. By 
> >>unconditionally using the cached edid from ioctls you break this use 
> >>case. Yes, such machines are broken, but we need to keep them working anyway.
> >>
> >>Also in my experience all machines are affected, we have examples 
> >>covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt 
> >>yet since we don't rely on the hpd bits any more (and so won't see bug reports any more).
> >>
> >>Generally if you use the hpd stuff your code must be designed under 
> >>the assumption that hpd is completely unreliably. We've seen 
> >>anything from random noise, flat-out not-working at all, stuck bits 
> >>and unstable hpd values that occasionally flip-flop. So you can't rely on it at all.
> >>
> >>> Note: In this same patch, there is additional optimization, which 
> >>> you pointed out,
> >>where we check if the connector->status is same as live status.
> >>> This can be removed independently, as you suggested.
> >>
> >>Hm, where have I pointed this out? Some other mail on internal discussions?
> >>
> >>> About patch 1:
> >>> We have done some local experiments and we came to know that for 
> >>> VLV and
> >>HSW boards, we can rely on the live status, if we give it some time 
> >>to settle (~300ms).
> >>> Probably, we need to modify this patch, as you suggested, until it 
> >>> becomes
> >>handy to be used reliably. We are on it, and will send another patch soon.
> >>>
> >>> But if somehow we are able to get some consistent results from 
> >>> live status, do
> >>you think it would be worth accepting this change, so that it can 
> >>handle soft HPDs and automation testing.
> >>> Because I believe we will face this problem whenever we are trying 
> >>> to test
> >>something from automation, where the physical device is not removed, 
> >>and DDC channel is up always.
> >>
> >>It's very well possible that all the platforms you have, but 
> >>experience says that some OEM will horrible screw this up. At least 
> >>they've consistently botched this in the past on occasional machines.
> >>
> >>Now the ghost hdmi detection on slow removal is obviously not great, 
> >>but we can't use the hpd bits to fix this. One approach would be.
> >>1. Upon hpd interrupt do an immediate probe of the connector. This 
> >>way we'll have good userspace experience if the unplug happens quickly and the hw works.
> >>2. Re-probe with a 1s delay to catch slow-uplugs. The current output 
> >>probing helpers are clever enough already that if a state-change 
> >>happens to be detected a uevent will be generate, irrespective of 
> >>the source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).
> >>
> >>Note that we already track the hpd interrupts on a per-source basis, 
> >>so doing the re-poll shouldn't be costly. Maybe do the re-poll as 
> >>part of the EDID invalidation to avoid stalling userspace.
> >>
> >>But you can't rely upon the hpd pins unfortunately :(
> >>
> >>This way we should be able to implement the 2 features you want, 
> >>even on unreliable hw.
> >>
> >>Cheers, Daniel
> >>--
> >>Daniel Vetter
> >>Software Engineer, Intel Corporation
> >>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >>_______________________________________________
> >>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
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Kumar, Shobhit]

On 4/11/2014 8:18 PM, Sharma, Shashank wrote:
> Ok, I will change the implementation.
>
> Regards
> Shashank
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Friday, April 11, 2014 7:53 PM
> To: Sharma, Shashank
> Cc: Daniel Vetter; C, Ramalingam; Wang, Quanxian; intel-gfx
> Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
>
> On Fri, Apr 11, 2014 at 01:23:43PM +0000, Sharma, Shashank wrote:
>> Thanks for the comments,
>> Actually, we are not using live_status at all.
>>
>> The check for < gen6 is only for EDID caching. So if the HW is >= gen6 cache_edid.
>> Else do not cache EDID, so that we will not block any of the old HW, which might not be HPD capable.
>
> Oh, I've thought that this is incremental on top of something you already have.
>>
>> Does it sound ok now :) ?
>
> No. HPD is _NOT_ I repeat _NOT_ reliably. Not even on gen6+. live status simply reflects the hpd pin, if either doesn't work, then neither does the other one.
>
> Nacked-with-prejudice-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Again re-starting the thread on this. Discussed this with Daniel on IRC 
and want to put the design agreed here for any further comments if any -

On detect call if no edid cached, then read full edid and cache. 
Assumption is that usermode will do the processing within 1s and we can 
just maintain this cache for 1s and clear. This will work also for 
panels which do not reliably assert HPD/Live status and also we get 
benefit of caching.

This will still fail compliance where we should not read any thing on 
ddc if live status is not detected. So plan is to add a compliance mode 
by way of module param which will be by default off. This code path will 
rely on HPD and Live status.

Regards
Shobhit