[PATCH 0/2] HDMI detect optimization and support for HDMI compliance

[PATCH 0/2] HDMI detect optimization and support for HDMI compliance

[shashank.sharma]

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

This patch set adds 2 patches:
1. drm/i915: Optimize HDMI EDID reads
This patch adds a EDID caching solution in intel_hdmi_detect function
to avoid multiple EDID reads for single HPD. A delayed work function
makes sure that the cached EDID gets clear after a time out.

2. drm/i915: Support for HDMI complaince HPD
This patch adds support for HDMI analyzers. Most of the analyzers send a
soft HPD event, where the HDMI cable removal is not required. In such 
scenarios, DDC is always up so EDID is always available, for both HPD
up and down. So we have to rely on live status register to check the 
actual HPD status. 

Shashank Sharma (2):
  drm/i915: Optimize HDMI EDID reads
  drm/i915: Support for HDMI complaince HPD

 drivers/gpu/drm/i915/i915_drv.h    |   1 +
 drivers/gpu/drm/i915/i915_params.c |   6 ++
 drivers/gpu/drm/i915/intel_drv.h   |   5 +
 drivers/gpu/drm/i915/intel_hdmi.c  | 181 +++++++++++++++++++++++++++++++++++--
 4 files changed, 186 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH 1/2] drm/i915: Optimize HDMI EDID reads

[shashank.sharma]

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

The current hdmi_detect() function is getting called from
many places, few of these are:
1. HDMI hot plug interrupt bottom half
2. get_resources() IOCTL family
3. drm_helper_probe_single_connector_modes() family
4. output_poll_execute()
5. status_show() etc...

Every time this function is called, it goes and reads HDMI EDID over
DDC channel. Ideally, reading EDID is only required when there is a
real hot plug, and then for all IOCTL and userspace detect functions
can be answered using this same EDID.

The current patch adds EDID caching for a finite duration (1 minute)
This is how it works:
1. With in this caching duration, when usespace get_resource and other
   modeset_detect calls get called, we can use the cached EDID.
2. Even the get_mode function can use the cached EDID.
3. A delayed work will clear the cached EDID after the timeout.
4. If there is a real HDMI hotplug, within the caching duration, the
   cached EDID is updates, and a new delayed work is scheduled.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |  4 ++
 drivers/gpu/drm/i915/intel_hdmi.c | 92 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 90 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 28d185d..185a45a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -110,6 +110,8 @@
 #define INTEL_DSI_VIDEO_MODE	0
 #define INTEL_DSI_COMMAND_MODE	1
 
+#define INTEL_HDMI_EDID_CACHING_SEC 60
+
 struct intel_framebuffer {
 	struct drm_framebuffer base;
 	struct drm_i915_gem_object *obj;
@@ -507,6 +509,8 @@ struct intel_hdmi {
 	enum hdmi_force_audio force_audio;
 	bool rgb_quant_range_selectable;
 	enum hdmi_picture_aspect aspect_ratio;
+	struct edid *edid;
+	struct delayed_work edid_work;
 	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 5f47d35..8dc3970 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -962,6 +962,22 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	return true;
 }
 
+/* Work function to invalidate EDID caching */
+static void intel_hdmi_invalidate_edid(struct work_struct *work)
+{
+	struct intel_hdmi *intel_hdmi = container_of(to_delayed_work(work),
+				struct intel_hdmi, edid_work);
+	struct drm_device *dev = intel_hdmi_to_dev(intel_hdmi);
+	struct drm_mode_config *mode_config = &dev->mode_config;
+
+	mutex_lock(&mode_config->mutex);
+	/* Checkpatch says kfree is NULL protected */
+	kfree(intel_hdmi->edid);
+	intel_hdmi->edid = NULL;
+	mutex_unlock(&mode_config->mutex);
+	DRM_DEBUG_DRIVER("cleaned up cached EDID\n");
+}
+
 static enum drm_connector_status
 intel_hdmi_detect(struct drm_connector *connector, bool force)
 {
@@ -978,15 +994,58 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
 
+	/*
+	* hdmi_detect() gets called from both get_resource()
+	* and HDMI hpd bottom half work function.
+	* Its not required to read EDID for every detect call until it's is
+	* from a hot plug. Lets cache the EDID as soon as we get
+	* a HPD, and then try to re-use this for all the non hpd detact calls
+	* coming with in a finite duration.
+	*/
+	if (INTEL_INFO(dev)->gen < 6)
+		/* Do not break old platforms */
+		goto skip_optimization;
+
+	/* If call is from HPD, do check real status by reading EDID */
+	if (!force)
+		goto skip_optimization;
+
+	/* This is a non-hpd call, lets see if we can optimize this */
+	if (intel_hdmi->edid) {
+		/*
+		* So this is a non-hpd call, within the duration when
+		* EDID caching is valid. So previous status (valid)
+		* of connector is re-usable.
+		*/
+		if (connector->status != connector_status_unknown) {
+			DRM_DEBUG_DRIVER("Reporting force status\n");
+			return connector->status;
+		}
+	}
+
+skip_optimization:
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
 	intel_hdmi->has_hdmi_sink = false;
 	intel_hdmi->has_audio = false;
 	intel_hdmi->rgb_quant_range_selectable = false;
+
+	/*
+	* You are well deserving, dear code, as you have survived
+	* all the optimizations. Now go and enjoy reading EDID
+	*/
 	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
+			intel_gmbus_get_adapter(dev_priv,
+						intel_hdmi->ddc_bus));
+	/*
+	* Now when we have read new EDID, update cached EDID with
+	* latest (both NULL or non NULL). Cancel the delayed work
+	* which cleans up the cached EDID. Re-schedule if required.
+	*/
+	kfree(intel_hdmi->edid);
+	intel_hdmi->edid = edid;
+	cancel_delayed_work_sync(&intel_hdmi->edid_work);
 
 	if (edid) {
 		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
@@ -997,8 +1056,17 @@ 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);
+			/*
+			* Allow re-use of cached EDID for 60 sec, as
+			* userspace modeset should happen within this
+			* duration, and multiple detect calls will be
+			* handled using cached EDID.
+			*/
+			schedule_delayed_work(&intel_hdmi->edid_work,
+				msecs_to_jiffies(
+					INTEL_HDMI_EDID_CACHING_SEC
+							* 1000));
 		}
-		kfree(edid);
 	}
 
 	if (status == connector_status_connected) {
@@ -1027,13 +1095,22 @@ static int intel_hdmi_get_modes(struct drm_connector *connector)
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
-
-	ret = intel_ddc_get_modes(connector,
+	/*
+	* GEN6 and + have software support for EDID caching, so
+	* use cached_edid from detect call, if available.
+	*/
+	if (intel_hdmi->edid && (INTEL_INFO(connector->dev)->gen >= 6)) {
+		ret = intel_connector_update_modes(connector,
+				intel_hdmi->edid);
+		DRM_DEBUG_DRIVER("Using cached EDID, got %d modes\n", ret);
+	} else {
+		ret = intel_ddc_get_modes(connector,
 				   intel_gmbus_get_adapter(dev_priv,
 							   intel_hdmi->ddc_bus));
+		DRM_DEBUG_DRIVER("Read EDID, got %d modes\n", ret);
+	}
 
 	intel_display_power_put(dev_priv, power_domain);
-
 	return ret;
 }
 
@@ -1661,5 +1738,8 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
 	intel_dig_port->dp.output_reg = 0;
 
+	/* Work function to invalidate cached EDID after timeout */
+	INIT_DELAYED_WORK(&(intel_dig_port->hdmi.edid_work),
+				intel_hdmi_invalidate_edid);
 	intel_hdmi_init_connector(intel_dig_port, intel_connector);
 }
-- 
1.9.1

[PATCH 2/2] drm/i915: Support for HDMI complaince HPD

[shashank.sharma]

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

During the HDMI complaince tests, most of the HDMI analyzers
issue a soft HPD, to automate the tests. This process keeps
the HDMI cable connected, and DDC chhanel alive.

HDMI detect() logic relies on EDID readability, to decide if
its a HDMI connect or disconnect event. But in this case, the
EDID is always readable, as HDMI cable is physically connected
and DDC channel is UP, so detect() always reports a HDMI connect
even if its intended to be a HDMI disconnect call.

So if HDMI compliance is enabled, we should rely on the live status
register, than EDID availability. This patch adds:
1. One kernel command line parameter for i915 module, which indicates
   if we want to support HDMI compliance, for this platform.
2. A new function to read EDID, which gets called only in case of
   HDMI compliance support is required. This function reads EDID only
   if live status register is up. The normal code flow doesn't get effected
   if kernel command line parameter is not enabled.
3. After various experiments on VLV2 board, with various HDMI monitors
   we have seen that, with few monitors, the live status register gets
   set after a slight delay, and then stays reliably. To support such
   monitors, there is a busy-loop added, with a max delay upto 50ms,
   with a status check after every 10ms. Please see the comment in
   intel_hdmi_get_edid.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |  1 +
 drivers/gpu/drm/i915/i915_params.c |  6 +++
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c  | 91 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 97 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2a372f2..19e4f97 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2162,6 +2162,7 @@ struct i915_params {
 	bool disable_vtd_wa;
 	int use_mmio_flip;
 	bool mmio_debug;
+	bool hdmi_compliance_support;
 };
 extern struct i915_params i915 __read_mostly;
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 62ee830..27dcc1a 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -50,6 +50,7 @@ struct i915_params i915 __read_mostly = {
 	.disable_vtd_wa = 0,
 	.use_mmio_flip = 0,
 	.mmio_debug = 0,
+	.hdmi_compliance_support = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -167,3 +168,8 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
 MODULE_PARM_DESC(mmio_debug,
 	"Enable the MMIO debug code (default: false). This may negatively "
 	"affect performance.");
+
+module_param_named(hdmi_compliance_support, i915.hdmi_compliance_support,
+							bool, 0400);
+MODULE_PARM_DESC(hdmi_compliance_support,
+		 "Support for HDMI compliance in i915 driver 1=On 0=Off");
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 185a45a..0b3798a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -111,6 +111,7 @@
 #define INTEL_DSI_COMMAND_MODE	1
 
 #define INTEL_HDMI_EDID_CACHING_SEC 60
+#define INTEL_HDMI_LIVE_STATUS_DELAY_MS 10
 
 struct intel_framebuffer {
 	struct drm_framebuffer base;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 8dc3970..06eb9de 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -978,6 +978,90 @@ static void intel_hdmi_invalidate_edid(struct work_struct *work)
 	DRM_DEBUG_DRIVER("cleaned up cached EDID\n");
 }
 
+/* Get HDMI live status */
+static bool intel_hdmi_live_status(struct drm_device *dev,
+			struct intel_hdmi *intel_hdmi)
+{
+	uint32_t status_bit;
+	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\n");
+
+	/* Live status bit is 29 for PORT_B, 28 for C and 27 for D */
+	status_bit = (PORTB_HOTPLUG_LIVE_STATUS_VLV >>
+		(PORT_B - intel_dig_port->port));
+
+	return I915_READ(PORT_HOTPLUG_STAT) & status_bit;
+}
+
+/* Read DDC and get EDID */
+struct edid *intel_hdmi_get_edid(struct drm_connector *connector, bool force)
+{
+	bool live_status = false;
+	struct edid *new_edid = NULL;
+	struct i2c_adapter *adapter;
+	struct drm_device *dev = connector->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
+	u8 retry = 5;
+
+	if (!intel_hdmi) {
+		DRM_ERROR("Invalid input to get hdmi\n");
+		return NULL;
+	}
+
+	/*
+	* Read EDID only when live status is up.
+	* This condition is required for HDMI analyzers, where
+	* there is no physical disconnect of HDMI cable, but
+	* only a HPD is issued. In this case, if we decide the status of
+	* HDMI based on EDID availability from DDC channel, its always
+	* available. So rely on live status.
+	*/
+	while (retry--) {
+		/*
+		* Many HDMI monitors set the live status after some delay
+		* Add some tolerance for them. From few experiments on VLV
+		* with a set of monitors, we got to know that a delay close to
+		* 50 ms can cover this gap.
+		* Retry reading live status 5 times, in 10ms duration each
+		* But make sure that you be well within limit of a HDCP unplug
+		* which can be as small as 100ms
+		*/
+		live_status = intel_hdmi_live_status(dev, intel_hdmi);
+		if (!live_status)
+			mdelay(INTEL_HDMI_LIVE_STATUS_DELAY_MS);
+		else
+			break;
+	}
+
+	/*
+	* Read EDID only if live status is up OR we are forced
+	* with a knife
+	*/
+	if (live_status || force) {
+		DRM_DEBUG_DRIVER("HDMI Live status is up\n");
+		adapter = intel_gmbus_get_adapter(dev_priv,
+		intel_hdmi->ddc_bus);
+
+		if (!adapter) {
+			DRM_ERROR("Get_hdmi cant get adapter\n");
+			return NULL;
+		}
+
+		new_edid = drm_get_edid(connector, adapter);
+		if (!new_edid) {
+			DRM_ERROR("Get_hdmi cant read edid\n");
+			return NULL;
+		}
+
+		DRM_DEBUG_KMS("Live status up, got EDID");
+	}
+	return new_edid;
+}
+
 static enum drm_connector_status
 intel_hdmi_detect(struct drm_connector *connector, bool force)
 {
@@ -1035,9 +1119,12 @@ skip_optimization:
 	* You are well deserving, dear code, as you have survived
 	* all the optimizations. Now go and enjoy reading EDID
 	*/
-	edid = drm_get_edid(connector,
+	if (i915.hdmi_compliance_support && (INTEL_INFO(dev)->gen >= 6))
+		edid = intel_hdmi_get_edid(connector, false);
+	else
+		edid = drm_get_edid(connector,
 			intel_gmbus_get_adapter(dev_priv,
-						intel_hdmi->ddc_bus));
+							intel_hdmi->ddc_bus));
 	/*
 	* Now when we have read new EDID, update cached EDID with
 	* latest (both NULL or non NULL). Cancel the delayed work
-- 
1.9.1

Re: [PATCH 2/2] drm/i915: Support for HDMI complaince HPD

[Chris Wilson]

On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sharma@intel.com wrote:
> From: Shashank Sharma <shashank.sharma@intel.com>
> 
> During the HDMI complaince tests, most of the HDMI analyzers
> issue a soft HPD, to automate the tests. This process keeps
> the HDMI cable connected, and DDC chhanel alive.
> 
> HDMI detect() logic relies on EDID readability, to decide if
> its a HDMI connect or disconnect event. But in this case, the
> EDID is always readable, as HDMI cable is physically connected
> and DDC channel is UP, so detect() always reports a HDMI connect
> even if its intended to be a HDMI disconnect call.
> 
> So if HDMI compliance is enabled, we should rely on the live status
> register, than EDID availability. This patch adds:
> 1. One kernel command line parameter for i915 module, which indicates
>    if we want to support HDMI compliance, for this platform.

I would rather have this as an output property. In fact, I would like
the hotplug detection method exposed (and even selectable, but other
than this compliance testing, I can't think of a scenario where the
kernel shouldn't be able to figure things out for itself).

> 2. A new function to read EDID, which gets called only in case of
>    HDMI compliance support is required. This function reads EDID only
>    if live status register is up. The normal code flow doesn't get effected
>    if kernel command line parameter is not enabled.
> 3. After various experiments on VLV2 board, with various HDMI monitors
>    we have seen that, with few monitors, the live status register gets
>    set after a slight delay, and then stays reliably. To support such
>    monitors, there is a busy-loop added, with a max delay upto 50ms,
>    with a status check after every 10ms. Please see the comment in
>    intel_hdmi_get_edid.

Wouldn't a tidier solution be to delay the hpd by 50-100ms after the
hotplug interrupt? That may overcome the issue with the live status for
all connectors...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Support for HDMI complaince HPD

[Sharma, Shashank]

Hello Chris,

Thanks for your time and comments.
I would like to give a brief history of the patch.

We tried to apply this optimization by default, and check all the EDID 
read based on the live status. But not all developers agreed to have 
this by default, with following reasons:
1. live_status was not very reliable for all platforms, so live_status
    based solution shouldn't be added.
2. they dint want EDID caching to be by default, as few old platforms
    were not even HPD capable.

So we came up with this intermediate solution to have:
1. Timeout based EDID caching, where cached EDID will be cleared after
    one minute.
2. An alternative code flow to support HDMI compliance (will be based
    on the live_status check, and will be only enabled for commercial
    platforms like android) which need HDMI compliance support. So the
    kernel command line parameter is to support the need to add this
    alternative EDID read method.

Please le me know your opinion about this, considering the background.

Regards
Shashank
On 8/12/2014 6:17 PM, Chris Wilson wrote:
> On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sharma@intel.com wrote:
>> From: Shashank Sharma <shashank.sharma@intel.com>
>>
>> During the HDMI complaince tests, most of the HDMI analyzers
>> issue a soft HPD, to automate the tests. This process keeps
>> the HDMI cable connected, and DDC chhanel alive.
>>
>> HDMI detect() logic relies on EDID readability, to decide if
>> its a HDMI connect or disconnect event. But in this case, the
>> EDID is always readable, as HDMI cable is physically connected
>> and DDC channel is UP, so detect() always reports a HDMI connect
>> even if its intended to be a HDMI disconnect call.
>>
>> So if HDMI compliance is enabled, we should rely on the live status
>> register, than EDID availability. This patch adds:
>> 1. One kernel command line parameter for i915 module, which indicates
>>     if we want to support HDMI compliance, for this platform.
>
> I would rather have this as an output property. In fact, I would like
> the hotplug detection method exposed (and even selectable, but other
> than this compliance testing, I can't think of a scenario where the
> kernel shouldn't be able to figure things out for itself).
>
>> 2. A new function to read EDID, which gets called only in case of
>>     HDMI compliance support is required. This function reads EDID only
>>     if live status register is up. The normal code flow doesn't get effected
>>     if kernel command line parameter is not enabled.
>> 3. After various experiments on VLV2 board, with various HDMI monitors
>>     we have seen that, with few monitors, the live status register gets
>>     set after a slight delay, and then stays reliably. To support such
>>     monitors, there is a busy-loop added, with a max delay upto 50ms,
>>     with a status check after every 10ms. Please see the comment in
>>     intel_hdmi_get_edid.
>
> Wouldn't a tidier solution be to delay the hpd by 50-100ms after the
> hotplug interrupt? That may overcome the issue with the live status for
> all connectors...
> -Chris
>

Re: [PATCH 2/2] drm/i915: Support for HDMI complaince HPD

[Chris Wilson]

On Tue, Aug 12, 2014 at 08:56:44PM +0530, Sharma, Shashank wrote:
> Hello Chris,
> 
> Thanks for your time and comments.
> I would like to give a brief history of the patch.
> 
> We tried to apply this optimization by default, and check all the
> EDID read based on the live status. But not all developers agreed to
> have this by default, with following reasons:
> 1. live_status was not very reliable for all platforms, so live_status
>    based solution shouldn't be added.
> 2. they dint want EDID caching to be by default, as few old platforms
>    were not even HPD capable.
> 
> So we came up with this intermediate solution to have:
> 1. Timeout based EDID caching, where cached EDID will be cleared after
>    one minute.
> 2. An alternative code flow to support HDMI compliance (will be based
>    on the live_status check, and will be only enabled for commercial
>    platforms like android) which need HDMI compliance support. So the
>    kernel command line parameter is to support the need to add this
>    alternative EDID read method.
> 
> Please le me know your opinion about this, considering the background.

I know. That is orthogonal to the tweaks I was suggesting. Also if you
feel you need to add details to your rationale, then your changelog is
incomplete.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Support for HDMI complaince HPD

[Sharma, Shashank]

 > I know. That is orthogonal to the tweaks I was suggesting. Also if you
 > feel you need to add details to your rationale, then your changelog is
 > incomplete.
 > -Chris

Thanks Chris,
Please find my comments inline to your previous mail, with suggestions.

On 8/12/2014 6:17 PM, Chris Wilson wrote:
> On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sharma@intel.com wrote:
>> From: Shashank Sharma <shashank.sharma@intel.com>
>>
>> During the HDMI complaince tests, most of the HDMI analyzers
>> issue a soft HPD, to automate the tests. This process keeps
>> the HDMI cable connected, and DDC chhanel alive.
>>
>> HDMI detect() logic relies on EDID readability, to decide if
>> its a HDMI connect or disconnect event. But in this case, the
>> EDID is always readable, as HDMI cable is physically connected
>> and DDC channel is UP, so detect() always reports a HDMI connect
>> even if its intended to be a HDMI disconnect call.
>>
>> So if HDMI compliance is enabled, we should rely on the live status
>> register, than EDID availability. This patch adds:
>> 1. One kernel command line parameter for i915 module, which indicates
>>     if we want to support HDMI compliance, for this platform.
>
> I would rather have this as an output property. In fact, I would like
> the hotplug detection method exposed (and even selectable, but other
> than this compliance testing, I can't think of a scenario where the
> kernel shouldn't be able to figure things out for itself).
Considering the history of the case, can you please elaborate this 
suggestion ? I dont think I am getting it right.
>
>> 2. A new function to read EDID, which gets called only in case of
>>     HDMI compliance support is required. This function reads EDID only
>>     if live status register is up. The normal code flow doesn't get effected
>>     if kernel command line parameter is not enabled.
>> 3. After various experiments on VLV2 board, with various HDMI monitors
>>     we have seen that, with few monitors, the live status register gets
>>     set after a slight delay, and then stays reliably. To support such
>>     monitors, there is a busy-loop added, with a max delay upto 50ms,
>>     with a status check after every 10ms. Please see the comment in
>>     intel_hdmi_get_edid.
>
> Wouldn't a tidier solution be to delay the hpd by 50-100ms after the
> hotplug interrupt? That may overcome the issue with the live status for
> all connectors...
> -Chris
>
There would be few problems in this case:
1. We dont want this scenario to come into picture for DP, as DP HPD
    pulse can be as small as 2ms.
2. Not all the HDMI monitors show this problem, but a significant
    subset of popular monitors do.
3. In HDCP compliance testing, we send a HPD pulse train of UP and
    Down, where down pulse can be as small as 100ms. If we increase the
    delay by 100ms, we will definitely miss the HPD down pulse.
4. The method what we are using is a busy waiting check, where we delay
    the pulse for 50ms, but take a sample of live_status per 10ms, so if
    the live status is up with a delay of 20ms, we needn't to waste
    another 30.
5. We want this code routine only to be executed for commercial (like
    android) platforms, whereas others get the routine code.

@ Danvet: Do you want to add something here ?

Regards
Shashank

Re: [PATCH 2/2] drm/i915: Support for HDMI complaince HPD

[Chris Wilson]

On Wed, Aug 13, 2014 at 11:34:02AM +0530, Sharma, Shashank wrote:
> > I know. That is orthogonal to the tweaks I was suggesting. Also if you
> > feel you need to add details to your rationale, then your changelog is
> > incomplete.
> > -Chris
> 
> Thanks Chris,
> Please find my comments inline to your previous mail, with suggestions.
> 
> On 8/12/2014 6:17 PM, Chris Wilson wrote:
> >On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sharma@intel.com wrote:
> >>From: Shashank Sharma <shashank.sharma@intel.com>
> >>
> >>During the HDMI complaince tests, most of the HDMI analyzers
> >>issue a soft HPD, to automate the tests. This process keeps
> >>the HDMI cable connected, and DDC chhanel alive.
> >>
> >>HDMI detect() logic relies on EDID readability, to decide if
> >>its a HDMI connect or disconnect event. But in this case, the
> >>EDID is always readable, as HDMI cable is physically connected
> >>and DDC channel is UP, so detect() always reports a HDMI connect
> >>even if its intended to be a HDMI disconnect call.
> >>
> >>So if HDMI compliance is enabled, we should rely on the live status
> >>register, than EDID availability. This patch adds:
> >>1. One kernel command line parameter for i915 module, which indicates
> >>    if we want to support HDMI compliance, for this platform.
> >
> >I would rather have this as an output property. In fact, I would like
> >the hotplug detection method exposed (and even selectable, but other
> >than this compliance testing, I can't think of a scenario where the
> >kernel shouldn't be able to figure things out for itself).
> Considering the history of the case, can you please elaborate this
> suggestion ? I dont think I am getting it right.

Instead of (or in addition to) adding a kernel parameter, you add an
output property so that it can be adjusted on the fly for individual
monitors.

> >
> >>2. A new function to read EDID, which gets called only in case of
> >>    HDMI compliance support is required. This function reads EDID only
> >>    if live status register is up. The normal code flow doesn't get effected
> >>    if kernel command line parameter is not enabled.
> >>3. After various experiments on VLV2 board, with various HDMI monitors
> >>    we have seen that, with few monitors, the live status register gets
> >>    set after a slight delay, and then stays reliably. To support such
> >>    monitors, there is a busy-loop added, with a max delay upto 50ms,
> >>    with a status check after every 10ms. Please see the comment in
> >>    intel_hdmi_get_edid.
> >
> >Wouldn't a tidier solution be to delay the hpd by 50-100ms after the
> >hotplug interrupt? That may overcome the issue with the live status for
> >all connectors...
> >-Chris
> >
> There would be few problems in this case:
> 1. We dont want this scenario to come into picture for DP, as DP HPD
>    pulse can be as small as 2ms.

If the live status is asserted and deasserted within 2ms, do you care?
Or perhaps you are talking about something else entirely.

> 2. Not all the HDMI monitors show this problem, but a significant
>    subset of popular monitors do.
> 3. In HDCP compliance testing, we send a HPD pulse train of UP and
>    Down, where down pulse can be as small as 100ms. If we increase the
>    delay by 100ms, we will definitely miss the HPD down pulse.

And? If the monitor is only plugged in for less than 0.1s do I really
want to waste 1-2s of user time reconfiguring the desktop and
applications before undoing all the changes?

There is no point in compliance testing if it does not actually test the
code going to be used.

> 4. The method what we are using is a busy waiting check, where we delay
>    the pulse for 50ms, but take a sample of live_status per 10ms, so if
>    the live status is up with a delay of 20ms, we needn't to waste
>    another 30.

Yes. You block using 100% of the cpu in an uninterruptable context for a
significant period of time. DO NOT DO THIS.

> 5. We want this code routine only to be executed for commercial (like
>    android) platforms, whereas others get the routine code.

In other words, you want to ignore years of real world compatibity testing
and larger user bases.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Support for HDMI complaince HPD

[Sharma, Shashank]

On 8/13/2014 11:46 AM, Chris Wilson wrote:
> On Wed, Aug 13, 2014 at 11:34:02AM +0530, Sharma, Shashank wrote:
>>> I know. That is orthogonal to the tweaks I was suggesting. Also if you
>>> feel you need to add details to your rationale, then your changelog is
>>> incomplete.
>>> -Chris
>>
>> Thanks Chris,
>> Please find my comments inline to your previous mail, with suggestions.
>>
>> On 8/12/2014 6:17 PM, Chris Wilson wrote:
>>> On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sharma@intel.com wrote:
>>>> From: Shashank Sharma <shashank.sharma@intel.com>
>>>>
>>>> During the HDMI complaince tests, most of the HDMI analyzers
>>>> issue a soft HPD, to automate the tests. This process keeps
>>>> the HDMI cable connected, and DDC chhanel alive.
>>>>
>>>> HDMI detect() logic relies on EDID readability, to decide if
>>>> its a HDMI connect or disconnect event. But in this case, the
>>>> EDID is always readable, as HDMI cable is physically connected
>>>> and DDC channel is UP, so detect() always reports a HDMI connect
>>>> even if its intended to be a HDMI disconnect call.
>>>>
>>>> So if HDMI compliance is enabled, we should rely on the live status
>>>> register, than EDID availability. This patch adds:
>>>> 1. One kernel command line parameter for i915 module, which indicates
>>>>     if we want to support HDMI compliance, for this platform.
>>>
>>> I would rather have this as an output property. In fact, I would like
>>> the hotplug detection method exposed (and even selectable, but other
>>> than this compliance testing, I can't think of a scenario where the
>>> kernel shouldn't be able to figure things out for itself).
>> Considering the history of the case, can you please elaborate this
>> suggestion ? I dont think I am getting it right.
>
> Instead of (or in addition to) adding a kernel parameter, you add an
> output property so that it can be adjusted on the fly for individual
> monitors.
>
Yes, this can be done. This might cause some problems with the first 
modeset from driver (like fb_console), where there is no userspace to 
set a property yet, but I think its manageable.
>>>
>>>> 2. A new function to read EDID, which gets called only in case of
>>>>     HDMI compliance support is required. This function reads EDID only
>>>>     if live status register is up. The normal code flow doesn't get effected
>>>>     if kernel command line parameter is not enabled.
>>>> 3. After various experiments on VLV2 board, with various HDMI monitors
>>>>     we have seen that, with few monitors, the live status register gets
>>>>     set after a slight delay, and then stays reliably. To support such
>>>>     monitors, there is a busy-loop added, with a max delay upto 50ms,
>>>>     with a status check after every 10ms. Please see the comment in
>>>>     intel_hdmi_get_edid.
>>>
>>> Wouldn't a tidier solution be to delay the hpd by 50-100ms after the
>>> hotplug interrupt? That may overcome the issue with the live status for
>>> all connectors...
>>> -Chris
>>>
>> There would be few problems in this case:
>> 1. We dont want this scenario to come into picture for DP, as DP HPD
>>     pulse can be as small as 2ms.
>
> If the live status is asserted and deasserted within 2ms, do you care?
> Or perhaps you are talking about something else entirely.
>
Actually what I mean was, in case of compliance, there would be an 
expectation for Port Up/Down/UP, but what we will get is UP/UP
This can fail the test case.	
>> 2. Not all the HDMI monitors show this problem, but a significant
>>     subset of popular monitors do.
>> 3. In HDCP compliance testing, we send a HPD pulse train of UP and
>>     Down, where down pulse can be as small as 100ms. If we increase the
>>     delay by 100ms, we will definitely miss the HPD down pulse.
>
> And? If the monitor is only plugged in for less than 0.1s do I really
> want to waste 1-2s of user time reconfiguring the desktop and
> applications before undoing all the changes?
>
> There is no point in compliance testing if it does not actually test the
> code going to be used.
>
I completely agree, but unfortunately if the test case fails, this would 
be blocker for commercial projects. The test clearly mentions we should 
respond properly to the HPD pulse train.
>> 4. The method what we are using is a busy waiting check, where we delay
>>     the pulse for 50ms, but take a sample of live_status per 10ms, so if
>>     the live status is up with a delay of 20ms, we needn't to waste
>>     another 30.
>
> Yes. You block using 100% of the cpu in an uninterruptable context for a
> significant period of time. DO NOT DO THIS.
>
I can very well switch to a sleep, but was not sure if the context 
switching 5 times for 10ms is beneficial :), please correct me if this 
is not ok.
>> 5. We want this code routine only to be executed for commercial (like
>>     android) platforms, whereas others get the routine code.
>
> In other words, you want to ignore years of real world compatibity testing
> and larger user bases.
> -Chris
>
I do not mean this for sure, in fact we would be happy if the community 
accepts this for regular code flow also, but due to their previous 
objections and stand for not to go for a live_status based solution, 
made to to come via this route. Please suggest how can we do this in 
better way.

Regards
Shashank

Re: [PATCH 2/2] drm/i915: Support for HDMI complaince HPD

[Daniel Vetter]

On Wed, Aug 13, 2014 at 01:12:12PM +0530, Sharma, Shashank wrote:
> On 8/13/2014 11:46 AM, Chris Wilson wrote:
> >On Wed, Aug 13, 2014 at 11:34:02AM +0530, Sharma, Shashank wrote:
> >>5. We want this code routine only to be executed for commercial (like
> >>    android) platforms, whereas others get the routine code.
> >
> >In other words, you want to ignore years of real world compatibity testing
> >and larger user bases.
> >
> I do not mean this for sure, in fact we would be happy if the community
> accepts this for regular code flow also, but due to their previous
> objections and stand for not to go for a live_status based solution, made to
> to come via this route. Please suggest how can we do this in better way.

So I'm grumpily ok with a special validation mode to get a sticker.
Occasionally there's a business need for those stickers, and it kinda
makes sense to merge that code upstream.

But in the end reality in the form of existing broken hdmi screens out
there wins. Always. And you actually learned that already by adding hacks
to your validation-only path to make it actually work in reality. Sooner
or later you need to add all the other crap we've accumulated too, or the
driver will simply not work. Which means we'll actually not end up with a
restricted validation-only hack, but duplicated code&bugs.

I'm _not_ going to have 2 separate paths with real-world quirks. If
there's anything actually broken with the current code in upstream, we
need to fix that one, not add a new one because that's easier. Because
it's not.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: Optimize HDMI EDID reads

[Daniel Vetter]

On Tue, Aug 12, 2014 at 06:08:20PM +0530, shashank.sharma@intel.com wrote:
> From: Shashank Sharma <shashank.sharma@intel.com>
> 
> The current hdmi_detect() function is getting called from
> many places, few of these are:
> 1. HDMI hot plug interrupt bottom half
> 2. get_resources() IOCTL family
> 3. drm_helper_probe_single_connector_modes() family
> 4. output_poll_execute()
> 5. status_show() etc...
> 
> Every time this function is called, it goes and reads HDMI EDID over
> DDC channel. Ideally, reading EDID is only required when there is a
> real hot plug, and then for all IOCTL and userspace detect functions
> can be answered using this same EDID.
> 
> The current patch adds EDID caching for a finite duration (1 minute)
> This is how it works:
> 1. With in this caching duration, when usespace get_resource and other
>    modeset_detect calls get called, we can use the cached EDID.
> 2. Even the get_mode function can use the cached EDID.
> 3. A delayed work will clear the cached EDID after the timeout.
> 4. If there is a real HDMI hotplug, within the caching duration, the
>    cached EDID is updates, and a new delayed work is scheduled.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>

This has a bunch of changes compared to what I've proposed, and so will
not actually work. Also, keying off the source platform (with the gen6
checks) is useless if we're dealing with random brokeness in existing hdmi
sinks here.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_drv.h  |  4 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 92 ++++++++++++++++++++++++++++++++++++---
>  2 files changed, 90 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 28d185d..185a45a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -110,6 +110,8 @@
>  #define INTEL_DSI_VIDEO_MODE	0
>  #define INTEL_DSI_COMMAND_MODE	1
>  
> +#define INTEL_HDMI_EDID_CACHING_SEC 60
> +
>  struct intel_framebuffer {
>  	struct drm_framebuffer base;
>  	struct drm_i915_gem_object *obj;
> @@ -507,6 +509,8 @@ struct intel_hdmi {
>  	enum hdmi_force_audio force_audio;
>  	bool rgb_quant_range_selectable;
>  	enum hdmi_picture_aspect aspect_ratio;
> +	struct edid *edid;
> +	struct delayed_work edid_work;
>  	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 5f47d35..8dc3970 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -962,6 +962,22 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	return true;
>  }
>  
> +/* Work function to invalidate EDID caching */
> +static void intel_hdmi_invalidate_edid(struct work_struct *work)
> +{
> +	struct intel_hdmi *intel_hdmi = container_of(to_delayed_work(work),
> +				struct intel_hdmi, edid_work);
> +	struct drm_device *dev = intel_hdmi_to_dev(intel_hdmi);
> +	struct drm_mode_config *mode_config = &dev->mode_config;
> +
> +	mutex_lock(&mode_config->mutex);
> +	/* Checkpatch says kfree is NULL protected */
> +	kfree(intel_hdmi->edid);
> +	intel_hdmi->edid = NULL;
> +	mutex_unlock(&mode_config->mutex);
> +	DRM_DEBUG_DRIVER("cleaned up cached EDID\n");
> +}
> +
>  static enum drm_connector_status
>  intel_hdmi_detect(struct drm_connector *connector, bool force)
>  {
> @@ -978,15 +994,58 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
>  
> +	/*
> +	* hdmi_detect() gets called from both get_resource()
> +	* and HDMI hpd bottom half work function.
> +	* Its not required to read EDID for every detect call until it's is
> +	* from a hot plug. Lets cache the EDID as soon as we get
> +	* a HPD, and then try to re-use this for all the non hpd detact calls
> +	* coming with in a finite duration.
> +	*/
> +	if (INTEL_INFO(dev)->gen < 6)
> +		/* Do not break old platforms */
> +		goto skip_optimization;
> +
> +	/* If call is from HPD, do check real status by reading EDID */
> +	if (!force)
> +		goto skip_optimization;
> +
> +	/* This is a non-hpd call, lets see if we can optimize this */
> +	if (intel_hdmi->edid) {
> +		/*
> +		* So this is a non-hpd call, within the duration when
> +		* EDID caching is valid. So previous status (valid)
> +		* of connector is re-usable.
> +		*/
> +		if (connector->status != connector_status_unknown) {
> +			DRM_DEBUG_DRIVER("Reporting force status\n");
> +			return connector->status;
> +		}
> +	}
> +
> +skip_optimization:
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
>  	intel_hdmi->has_hdmi_sink = false;
>  	intel_hdmi->has_audio = false;
>  	intel_hdmi->rgb_quant_range_selectable = false;
> +
> +	/*
> +	* You are well deserving, dear code, as you have survived
> +	* all the optimizations. Now go and enjoy reading EDID
> +	*/
>  	edid = drm_get_edid(connector,
> -			    intel_gmbus_get_adapter(dev_priv,
> -						    intel_hdmi->ddc_bus));
> +			intel_gmbus_get_adapter(dev_priv,
> +						intel_hdmi->ddc_bus));
> +	/*
> +	* Now when we have read new EDID, update cached EDID with
> +	* latest (both NULL or non NULL). Cancel the delayed work
> +	* which cleans up the cached EDID. Re-schedule if required.
> +	*/
> +	kfree(intel_hdmi->edid);
> +	intel_hdmi->edid = edid;
> +	cancel_delayed_work_sync(&intel_hdmi->edid_work);
>  
>  	if (edid) {
>  		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> @@ -997,8 +1056,17 @@ 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);
> +			/*
> +			* Allow re-use of cached EDID for 60 sec, as
> +			* userspace modeset should happen within this
> +			* duration, and multiple detect calls will be
> +			* handled using cached EDID.
> +			*/
> +			schedule_delayed_work(&intel_hdmi->edid_work,
> +				msecs_to_jiffies(
> +					INTEL_HDMI_EDID_CACHING_SEC
> +							* 1000));
>  		}
> -		kfree(edid);
>  	}
>  
>  	if (status == connector_status_connected) {
> @@ -1027,13 +1095,22 @@ static int intel_hdmi_get_modes(struct drm_connector *connector)
>  
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
> -
> -	ret = intel_ddc_get_modes(connector,
> +	/*
> +	* GEN6 and + have software support for EDID caching, so
> +	* use cached_edid from detect call, if available.
> +	*/
> +	if (intel_hdmi->edid && (INTEL_INFO(connector->dev)->gen >= 6)) {
> +		ret = intel_connector_update_modes(connector,
> +				intel_hdmi->edid);
> +		DRM_DEBUG_DRIVER("Using cached EDID, got %d modes\n", ret);
> +	} else {
> +		ret = intel_ddc_get_modes(connector,
>  				   intel_gmbus_get_adapter(dev_priv,
>  							   intel_hdmi->ddc_bus));
> +		DRM_DEBUG_DRIVER("Read EDID, got %d modes\n", ret);
> +	}
>  
>  	intel_display_power_put(dev_priv, power_domain);
> -
>  	return ret;
>  }
>  
> @@ -1661,5 +1738,8 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>  	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
>  	intel_dig_port->dp.output_reg = 0;
>  
> +	/* Work function to invalidate cached EDID after timeout */
> +	INIT_DELAYED_WORK(&(intel_dig_port->hdmi.edid_work),
> +				intel_hdmi_invalidate_edid);
>  	intel_hdmi_init_connector(intel_dig_port, intel_connector);
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> 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 1/2] drm/i915: Optimize HDMI EDID reads

[Sharma, Shashank]

Regards
Shashank
On 8/13/2014 5:44 PM, Daniel Vetter wrote:
> On Tue, Aug 12, 2014 at 06:08:20PM +0530, shashank.sharma@intel.com wrote:
>> From: Shashank Sharma <shashank.sharma@intel.com>
>>
>> The current hdmi_detect() function is getting called from
>> many places, few of these are:
>> 1. HDMI hot plug interrupt bottom half
>> 2. get_resources() IOCTL family
>> 3. drm_helper_probe_single_connector_modes() family
>> 4. output_poll_execute()
>> 5. status_show() etc...
>>
>> Every time this function is called, it goes and reads HDMI EDID over
>> DDC channel. Ideally, reading EDID is only required when there is a
>> real hot plug, and then for all IOCTL and userspace detect functions
>> can be answered using this same EDID.
>>
>> The current patch adds EDID caching for a finite duration (1 minute)
>> This is how it works:
>> 1. With in this caching duration, when usespace get_resource and other
>>     modeset_detect calls get called, we can use the cached EDID.
>> 2. Even the get_mode function can use the cached EDID.
>> 3. A delayed work will clear the cached EDID after the timeout.
>> 4. If there is a real HDMI hotplug, within the caching duration, the
>>     cached EDID is updates, and a new delayed work is scheduled.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>
> This has a bunch of changes compared to what I've proposed, and so will
> not actually work. Also, keying off the source platform (with the gen6
> checks) is useless if we're dealing with random brokeness in existing hdmi
> sinks here.
> -Daniel
>
Can you please point out what is it, that's unexpected to you ?
I think this is what we (you & Shobhit) agreed on:
1. Timeout based EDID caching
2. Use of cached EDID within caching duration
3. Separate path for HDMI compliance, controllable in kernel, which 
doesn't affect current code flow.

>> ---
>>   drivers/gpu/drm/i915/intel_drv.h  |  4 ++
>>   drivers/gpu/drm/i915/intel_hdmi.c | 92 ++++++++++++++++++++++++++++++++++++---
>>   2 files changed, 90 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 28d185d..185a45a 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -110,6 +110,8 @@
>>   #define INTEL_DSI_VIDEO_MODE	0
>>   #define INTEL_DSI_COMMAND_MODE	1
>>
>> +#define INTEL_HDMI_EDID_CACHING_SEC 60
>> +
>>   struct intel_framebuffer {
>>   	struct drm_framebuffer base;
>>   	struct drm_i915_gem_object *obj;
>> @@ -507,6 +509,8 @@ struct intel_hdmi {
>>   	enum hdmi_force_audio force_audio;
>>   	bool rgb_quant_range_selectable;
>>   	enum hdmi_picture_aspect aspect_ratio;
>> +	struct edid *edid;
>> +	struct delayed_work edid_work;
>>   	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 5f47d35..8dc3970 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -962,6 +962,22 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>   	return true;
>>   }
>>
>> +/* Work function to invalidate EDID caching */
>> +static void intel_hdmi_invalidate_edid(struct work_struct *work)
>> +{
>> +	struct intel_hdmi *intel_hdmi = container_of(to_delayed_work(work),
>> +				struct intel_hdmi, edid_work);
>> +	struct drm_device *dev = intel_hdmi_to_dev(intel_hdmi);
>> +	struct drm_mode_config *mode_config = &dev->mode_config;
>> +
>> +	mutex_lock(&mode_config->mutex);
>> +	/* Checkpatch says kfree is NULL protected */
>> +	kfree(intel_hdmi->edid);
>> +	intel_hdmi->edid = NULL;
>> +	mutex_unlock(&mode_config->mutex);
>> +	DRM_DEBUG_DRIVER("cleaned up cached EDID\n");
>> +}
>> +
>>   static enum drm_connector_status
>>   intel_hdmi_detect(struct drm_connector *connector, bool force)
>>   {
>> @@ -978,15 +994,58 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>>   	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>   		      connector->base.id, connector->name);
>>
>> +	/*
>> +	* hdmi_detect() gets called from both get_resource()
>> +	* and HDMI hpd bottom half work function.
>> +	* Its not required to read EDID for every detect call until it's is
>> +	* from a hot plug. Lets cache the EDID as soon as we get
>> +	* a HPD, and then try to re-use this for all the non hpd detact calls
>> +	* coming with in a finite duration.
>> +	*/
>> +	if (INTEL_INFO(dev)->gen < 6)
>> +		/* Do not break old platforms */
>> +		goto skip_optimization;
>> +
>> +	/* If call is from HPD, do check real status by reading EDID */
>> +	if (!force)
>> +		goto skip_optimization;
>> +
>> +	/* This is a non-hpd call, lets see if we can optimize this */
>> +	if (intel_hdmi->edid) {
>> +		/*
>> +		* So this is a non-hpd call, within the duration when
>> +		* EDID caching is valid. So previous status (valid)
>> +		* of connector is re-usable.
>> +		*/
>> +		if (connector->status != connector_status_unknown) {
>> +			DRM_DEBUG_DRIVER("Reporting force status\n");
>> +			return connector->status;
>> +		}
>> +	}
>> +
>> +skip_optimization:
>>   	power_domain = intel_display_port_power_domain(intel_encoder);
>>   	intel_display_power_get(dev_priv, power_domain);
>>
>>   	intel_hdmi->has_hdmi_sink = false;
>>   	intel_hdmi->has_audio = false;
>>   	intel_hdmi->rgb_quant_range_selectable = false;
>> +
>> +	/*
>> +	* You are well deserving, dear code, as you have survived
>> +	* all the optimizations. Now go and enjoy reading EDID
>> +	*/
>>   	edid = drm_get_edid(connector,
>> -			    intel_gmbus_get_adapter(dev_priv,
>> -						    intel_hdmi->ddc_bus));
>> +			intel_gmbus_get_adapter(dev_priv,
>> +						intel_hdmi->ddc_bus));
>> +	/*
>> +	* Now when we have read new EDID, update cached EDID with
>> +	* latest (both NULL or non NULL). Cancel the delayed work
>> +	* which cleans up the cached EDID. Re-schedule if required.
>> +	*/
>> +	kfree(intel_hdmi->edid);
>> +	intel_hdmi->edid = edid;
>> +	cancel_delayed_work_sync(&intel_hdmi->edid_work);
>>
>>   	if (edid) {
>>   		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
>> @@ -997,8 +1056,17 @@ 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);
>> +			/*
>> +			* Allow re-use of cached EDID for 60 sec, as
>> +			* userspace modeset should happen within this
>> +			* duration, and multiple detect calls will be
>> +			* handled using cached EDID.
>> +			*/
>> +			schedule_delayed_work(&intel_hdmi->edid_work,
>> +				msecs_to_jiffies(
>> +					INTEL_HDMI_EDID_CACHING_SEC
>> +							* 1000));
>>   		}
>> -		kfree(edid);
>>   	}
>>
>>   	if (status == connector_status_connected) {
>> @@ -1027,13 +1095,22 @@ static int intel_hdmi_get_modes(struct drm_connector *connector)
>>
>>   	power_domain = intel_display_port_power_domain(intel_encoder);
>>   	intel_display_power_get(dev_priv, power_domain);
>> -
>> -	ret = intel_ddc_get_modes(connector,
>> +	/*
>> +	* GEN6 and + have software support for EDID caching, so
>> +	* use cached_edid from detect call, if available.
>> +	*/
>> +	if (intel_hdmi->edid && (INTEL_INFO(connector->dev)->gen >= 6)) {
>> +		ret = intel_connector_update_modes(connector,
>> +				intel_hdmi->edid);
>> +		DRM_DEBUG_DRIVER("Using cached EDID, got %d modes\n", ret);
>> +	} else {
>> +		ret = intel_ddc_get_modes(connector,
>>   				   intel_gmbus_get_adapter(dev_priv,
>>   							   intel_hdmi->ddc_bus));
>> +		DRM_DEBUG_DRIVER("Read EDID, got %d modes\n", ret);
>> +	}
>>
>>   	intel_display_power_put(dev_priv, power_domain);
>> -
>>   	return ret;
>>   }
>>
>> @@ -1661,5 +1738,8 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>>   	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
>>   	intel_dig_port->dp.output_reg = 0;
>>
>> +	/* Work function to invalidate cached EDID after timeout */
>> +	INIT_DELAYED_WORK(&(intel_dig_port->hdmi.edid_work),
>> +				intel_hdmi_invalidate_edid);
>>   	intel_hdmi_init_connector(intel_dig_port, intel_connector);
>>   }
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Re: [PATCH 1/2] drm/i915: Optimize HDMI EDID reads

[Daniel Vetter]

On Thu, Aug 14, 2014 at 8:15 AM, Sharma, Shashank
<shashank.sharma@intel.com> wrote:
> Can you please point out what is it, that's unexpected to you ?
> I think this is what we (you & Shobhit) agreed on:
> 1. Timeout based EDID caching
> 2. Use of cached EDID within caching duration
> 3. Separate path for HDMI compliance, controllable in kernel, which doesn't
> affect current code flow.

- The timeout is 1 minute instead of 1s. That breaks interactions with
the periodical probing we do when there's a storm.
- There's a generation check in there. This is a generic problem,
restricting platforms only means that fewer people will be able to
test it and find issues with broken hdmi sinks. The problem here are
_sink_ devices, not necessarily platforms. So testing for platforms is
bogus.
- Keying off the force parameter isn't actually precise enough.
There's an encoder->hot_plug callback where you should invalidate the
edid instead.
- Adding the edid caching to the intel_hdmi struct is the wrong place,
we already have an edid pointer in intel_connector, which is used for
panels. Augmenting that to allow caching with time-based invalidation
is the right solution instead of inventing a completely new wheel.

Aside: You commit message is misleading since it's actually not
required to do a full probe cycle for the get_connector ioctl. You can
get at the current cached mode list without any probe calls at all.
Please have a look at SNA for how to do that. And if you have
userspace constantly probing sysfs files and other stuff instead of
listening to uevents then you need to fix your userspace, not cache
the edid in the kernel for a minute.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: Optimize HDMI EDID reads

[Sharma, Shashank]

Hi Daniel,

I can do all the changes accordingly and upload a new patch.

This is what I am going to do:
1. Change the EDID caching time to a second from a minute (probably 
there was a miscommunication).
2. Remove the gen_check
3. Use the connector->edid pointer to cache EDID.

I have only few problems with these two suggestions:
 > Keying off the force parameter isn't actually precise enough.
It is. All the calls to HDMI detect, which come as a result of user 
space interaction keep force = 1, whereas all the hot plug event callers 
keep it force = 0. Please have a look:

IOCTL / Sysfs calls, calling connector->funcs->detect()
1. drm_helper_probe_single_connector_modes_merge_bits => (force = 1)
2. status_show => (force = 1)

Hot plug handlers / hot plug poll
1. drm_helper_hpd_irq_event => (force = 0)
2. output_poll_execute => (force = 0)
So this should work all right.

 > There's an encoder->hot_plug callback where you should invalidate the
 > edid instead.
In MCG branch, we are doing this in encoder->hot_plug only. But there 
the EDID stays, until there is one more next hotplug, and by that time, 
the detect code uses cached EDID only.
As encoder->hot_plug function also gets called every time there is a 
hot_plug, from the hotplug_work_fn, I was afraid this might cause a 
race. Second, I still have to write a delayed_work wrapper, to call 
encoder->hot_plug from, after the timeout.

If you feel that, its better to do it there, I can do changes accordingly.

Regards
Shashank
On 8/14/2014 1:58 PM, Daniel Vetter wrote:
> On Thu, Aug 14, 2014 at 8:15 AM, Sharma, Shashank
> <shashank.sharma@intel.com> wrote:
>> Can you please point out what is it, that's unexpected to you ?
>> I think this is what we (you & Shobhit) agreed on:
>> 1. Timeout based EDID caching
>> 2. Use of cached EDID within caching duration
>> 3. Separate path for HDMI compliance, controllable in kernel, which doesn't
>> affect current code flow.
>
> - The timeout is 1 minute instead of 1s. That breaks interactions with
> the periodical probing we do when there's a storm.
> - There's a generation check in there. This is a generic problem,
> restricting platforms only means that fewer people will be able to
> test it and find issues with broken hdmi sinks. The problem here are
> _sink_ devices, not necessarily platforms. So testing for platforms is
> bogus.
> - Keying off the force parameter isn't actually precise enough.
> There's an encoder->hot_plug callback where you should invalidate the
> edid instead.
> - Adding the edid caching to the intel_hdmi struct is the wrong place,
> we already have an edid pointer in intel_connector, which is used for
> panels. Augmenting that to allow caching with time-based invalidation
> is the right solution instead of inventing a completely new wheel.
>
> Aside: You commit message is misleading since it's actually not
> required to do a full probe cycle for the get_connector ioctl. You can
> get at the current cached mode list without any probe calls at all.
> Please have a look at SNA for how to do that. And if you have
> userspace constantly probing sysfs files and other stuff instead of
> listening to uevents then you need to fix your userspace, not cache
> the edid in the kernel for a minute.
> -Daniel
>

Re: [PATCH 1/2] drm/i915: Optimize HDMI EDID reads

[Daniel Vetter]

On Thu, Aug 14, 2014 at 02:53:44PM +0530, Sharma, Shashank wrote:
> Hi Daniel,
> 
> I can do all the changes accordingly and upload a new patch.
> 
> This is what I am going to do:
> 1. Change the EDID caching time to a second from a minute (probably there
> was a miscommunication).
> 2. Remove the gen_check
> 3. Use the connector->edid pointer to cache EDID.
> 
> I have only few problems with these two suggestions:
> > Keying off the force parameter isn't actually precise enough.
> It is. All the calls to HDMI detect, which come as a result of user space
> interaction keep force = 1, whereas all the hot plug event callers keep it
> force = 0. Please have a look:
> 
> IOCTL / Sysfs calls, calling connector->funcs->detect()
> 1. drm_helper_probe_single_connector_modes_merge_bits => (force = 1)
> 2. status_show => (force = 1)
> 
> Hot plug handlers / hot plug poll
> 1. drm_helper_hpd_irq_event => (force = 0)
> 2. output_poll_execute => (force = 0)
> So this should work all right.

Hm indeed, I've mixed up things with the output poll worker. My concern is
mostly that "force" already has overloaded meaning. But I think if we
polish the code-flow a bit it should work out. Quick draft:

	/* at the top of the detect function before anything else */
	if (!force)
		intel_connector_invalidate_edid(conn);


Then we replace all current calls to drm_get_edid with a new
intel_connector_get_edid_cached, which first checks the conn->edid cache
and only if that's empty call drm_edid_cache.

btw for the edid cache itself I think Chris' idea to use errno pointers is
great, so the state machine would be:

connector->edid == NULL -> cache expired

IS_ERR(connector->edid) -> negative cache entry with errno value, e.g.
-ENXIO. this way we also speed up subsequent detect calls from userspace
when nothing is connected.

everything else -> cached edid

So in code this would look like

void intel_connector_invalidate_edid(conn)
{
	/* locking tbd */

	if (!conn->edid)
		return;

	if (!IS_ERR(conn->edid))
		kfree(conn->edid);

	conn->edid = NULL;
}

struct drm_edid *intel_connector_get_edid_cached(conn, ...)
{
	/* locking tbd */

	if (!conn->edid) {
		conn->edid = drm_get_edid(...);
		if (!conn->edid)
			conn->edid = ERR_PTR(-ENXIO);

		/* rearm invalidate timer */
	}

	return IS_ERR(conn->edid) ? NULL : conn->edid;
}

btw for locking I'd just pick a spinlock, then you can use a simple timer
to free the edid. Currently the drm modeset locking is a bit fuzzy around
connectors (due to the dp mst introduction), so separate locking would
simplify things.

> > There's an encoder->hot_plug callback where you should invalidate the
> > edid instead.
> In MCG branch, we are doing this in encoder->hot_plug only. But there the
> EDID stays, until there is one more next hotplug, and by that time, the
> detect code uses cached EDID only.
> As encoder->hot_plug function also gets called every time there is a
> hot_plug, from the hotplug_work_fn, I was afraid this might cause a race.
> Second, I still have to write a delayed_work wrapper, to call
> encoder->hot_plug from, after the timeout.

I've thought about the hotplug work function when we invalidate the edid,
and I think we can wait with that until there's demand. Currently if the
sink is horribly broken and does only respond after a while we will also
not detect it right away. And as long as we invalidate the edid soonish
(before the user could poke his system to figure out where the screen
went) we'll be good.

> If you feel that, its better to do it there, I can do changes accordingly.

Nah, I think if we wrap the caching/invalidation up into the above
suggested tiny helpers the code will be clear enough and your idea of
using "force" indeed works.

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

Re: [PATCH 1/2] drm/i915: Optimize HDMI EDID reads

[Sharma, Shashank]

Thanks Daniel.
Let me incorporate these changes you suggested, and prepare a new patch set. 

Regards
Shashank 
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Thursday, August 14, 2014 5:27 PM
To: Sharma, Shashank
Cc: Daniel Vetter; intel-gfx; Vetter, Daniel; Kumar, Shobhit
Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Optimize HDMI EDID reads

On Thu, Aug 14, 2014 at 02:53:44PM +0530, Sharma, Shashank wrote:
> Hi Daniel,
> 
> I can do all the changes accordingly and upload a new patch.
> 
> This is what I am going to do:
> 1. Change the EDID caching time to a second from a minute (probably 
> there was a miscommunication).
> 2. Remove the gen_check
> 3. Use the connector->edid pointer to cache EDID.
> 
> I have only few problems with these two suggestions:
> > Keying off the force parameter isn't actually precise enough.
> It is. All the calls to HDMI detect, which come as a result of user 
> space interaction keep force = 1, whereas all the hot plug event 
> callers keep it force = 0. Please have a look:
> 
> IOCTL / Sysfs calls, calling connector->funcs->detect() 1. 
> drm_helper_probe_single_connector_modes_merge_bits => (force = 1) 2. 
> status_show => (force = 1)
> 
> Hot plug handlers / hot plug poll
> 1. drm_helper_hpd_irq_event => (force = 0) 2. output_poll_execute => 
> (force = 0) So this should work all right.

Hm indeed, I've mixed up things with the output poll worker. My concern is mostly that "force" already has overloaded meaning. But I think if we polish the code-flow a bit it should work out. Quick draft:

	/* at the top of the detect function before anything else */
	if (!force)
		intel_connector_invalidate_edid(conn);


Then we replace all current calls to drm_get_edid with a new intel_connector_get_edid_cached, which first checks the conn->edid cache and only if that's empty call drm_edid_cache.

btw for the edid cache itself I think Chris' idea to use errno pointers is great, so the state machine would be:

connector->edid == NULL -> cache expired

IS_ERR(connector->edid) -> negative cache entry with errno value, e.g.
-ENXIO. this way we also speed up subsequent detect calls from userspace when nothing is connected.

everything else -> cached edid

So in code this would look like

void intel_connector_invalidate_edid(conn)
{
	/* locking tbd */

	if (!conn->edid)
		return;

	if (!IS_ERR(conn->edid))
		kfree(conn->edid);

	conn->edid = NULL;
}

struct drm_edid *intel_connector_get_edid_cached(conn, ...) {
	/* locking tbd */

	if (!conn->edid) {
		conn->edid = drm_get_edid(...);
		if (!conn->edid)
			conn->edid = ERR_PTR(-ENXIO);

		/* rearm invalidate timer */
	}

	return IS_ERR(conn->edid) ? NULL : conn->edid; }

btw for locking I'd just pick a spinlock, then you can use a simple timer to free the edid. Currently the drm modeset locking is a bit fuzzy around connectors (due to the dp mst introduction), so separate locking would simplify things.

> > There's an encoder->hot_plug callback where you should invalidate 
> > the edid instead.
> In MCG branch, we are doing this in encoder->hot_plug only. But there 
> the EDID stays, until there is one more next hotplug, and by that 
> time, the detect code uses cached EDID only.
> As encoder->hot_plug function also gets called every time there is a 
> hot_plug, from the hotplug_work_fn, I was afraid this might cause a race.
> Second, I still have to write a delayed_work wrapper, to call
> encoder->hot_plug from, after the timeout.

I've thought about the hotplug work function when we invalidate the edid, and I think we can wait with that until there's demand. Currently if the sink is horribly broken and does only respond after a while we will also not detect it right away. And as long as we invalidate the edid soonish (before the user could poke his system to figure out where the screen
went) we'll be good.

> If you feel that, its better to do it there, I can do changes accordingly.

Nah, I think if we wrap the caching/invalidation up into the above suggested tiny helpers the code will be clear enough and your idea of using "force" indeed works.

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