[PATCH] drm/i915: cache the EDID for eDP panels

[PATCH] drm/i915: cache the EDID for eDP panels

[Jesse Barnes]

They aren't going anywhere, and probing on DDC can cause the panel to
blank briefly, so read them up front and cache them for later queries.

v2: fix potential NULL derefs in intel_dp_get_edid_modes and
    intel_dp_get_edid (Jani)
    copy full EDID length, including extension blocks (Takashi)
    free EDID on teardown (Takashi)

References: https://bugs.freedesktop.org/show_bug.cgi?id=46856
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   45 +++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6538c46..3397251 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -32,6 +32,7 @@
 #include "drm.h"
 #include "drm_crtc.h"
 #include "drm_crtc_helper.h"
+#include "drm_edid.h"
 #include "intel_drv.h"
 #include "i915_drm.h"
 #include "i915_drv.h"
@@ -67,6 +68,8 @@ struct intel_dp {
 	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
+	struct edid *edid; /* cached EDID for eDP */
+	int edid_mode_count;
 };
 
 /**
@@ -2122,9 +2125,17 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct edid	*edid;
 
-	ironlake_edp_panel_vdd_on(intel_dp);
+	if (is_edp(intel_dp) && intel_dp->edid) {
+		edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
+		if (!edid)
+			return NULL;
+
+		memcpy(edid, intel_dp->edid, EDID_LENGTH *
+		       (intel_dp->edid->extensions + 1));
+		return edid;
+	}
+
 	edid = drm_get_edid(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return edid;
 }
 
@@ -2134,9 +2145,17 @@ intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *ada
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	int	ret;
 
-	ironlake_edp_panel_vdd_on(intel_dp);
+	if (is_edp(intel_dp) && intel_dp->edid) {
+		drm_mode_connector_update_edid_property(connector,
+							intel_dp->edid);
+		ret = drm_add_edid_modes(connector, intel_dp->edid);
+		drm_edid_to_eld(connector,
+				intel_dp->edid);
+		connector->display_info.raw_edid = NULL;
+		return intel_dp->edid_mode_count;
+	}
+
 	ret = intel_ddc_get_modes(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return ret;
 }
 
@@ -2326,6 +2345,7 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	i2c_del_adapter(&intel_dp->adapter);
 	drm_encoder_cleanup(encoder);
 	if (is_edp(intel_dp)) {
+		kfree(intel_dp->edid);
 		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
 		ironlake_panel_vdd_off_sync(intel_dp);
 	}
@@ -2509,11 +2529,14 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			break;
 	}
 
+	intel_dp_i2c_init(intel_dp, intel_connector, name);
+
 	/* Cache some DPCD data in the eDP case */
 	if (is_edp(intel_dp)) {
 		bool ret;
 		struct edp_power_seq	cur, vbt;
 		u32 pp_on, pp_off, pp_div;
+		struct edid *edid;
 
 		pp_on = I915_READ(PCH_PP_ON_DELAYS);
 		pp_off = I915_READ(PCH_PP_OFF_DELAYS);
@@ -2581,9 +2604,19 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			intel_dp_destroy(&intel_connector->base);
 			return;
 		}
-	}
 
-	intel_dp_i2c_init(intel_dp, intel_connector, name);
+		ironlake_edp_panel_vdd_on(intel_dp);
+		edid = drm_get_edid(connector, &intel_dp->adapter);
+		if (edid) {
+			drm_mode_connector_update_edid_property(connector,
+								edid);
+			intel_dp->edid_mode_count =
+				drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			intel_dp->edid = edid;
+		}
+		ironlake_edp_panel_vdd_off(intel_dp, false);
+	}
 
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
-- 
1.7.9.5

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Daniel Vetter]

On Fri, Jun 15, 2012 at 10:31:06AM -0700, Jesse Barnes wrote:
> On Fri, 15 Jun 2012 10:34:22 +0300
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > Hi Jesse, I'm sure you meant to do *something* with that return value. I
> > presume it should be equal to intel_dp->edid_mode_count, but is it
> > possible it's not? Is it better to return ret or
> > intel_dp->edid_mode_count from this function?
> > 
> 
> Heh there's always something... but yeah it should be equal to the
> mode_count; I should either drop the ret= or use it.  Maybe as a
> cleanup on top since I'm tired of respinning this one? :p

I guess we can mend that bikeshed when we unify the panel handling some
more between lvds and eDP. Patch merged for -fixes, thanks.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Jesse Barnes]

On Fri, 15 Jun 2012 10:34:22 +0300
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Thu, 14 Jun 2012, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > They aren't going anywhere, and probing on DDC can cause the panel to
> > blank briefly, so read them up front and cache them for later queries.
> >
> > v2: fix potential NULL derefs in intel_dp_get_edid_modes and
> >     intel_dp_get_edid (Jani)
> >     copy full EDID length, including extension blocks (Takashi)
> >     free EDID on teardown (Takashi)
> > v3: malloc a new EDID buffer that's big enough for the memcpy (Chris)
> > v4: change handling of NULL EDIDs, just preserve the NULL behavior
> >     across detects and mode list fetches rather than trying to re-fetch
> >     the EDID (Chris)
> > v5: be glad that Chris is around to remind me to hit C-x C-s before
> >     committing.
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=46856
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c |   49 ++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 43 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 6538c46..69d2f0c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -32,6 +32,7 @@
> >  #include "drm.h"
> >  #include "drm_crtc.h"
> >  #include "drm_crtc_helper.h"
> > +#include "drm_edid.h"
> >  #include "intel_drv.h"
> >  #include "i915_drm.h"
> >  #include "i915_drv.h"
> > @@ -67,6 +68,8 @@ struct intel_dp {
> >  	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
> >  	struct delayed_work panel_vdd_work;
> >  	bool want_panel_vdd;
> > +	struct edid *edid; /* cached EDID for eDP */
> > +	int edid_mode_count;
> >  };
> >  
> >  /**
> > @@ -2121,10 +2124,22 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> >  {
> >  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> >  	struct edid	*edid;
> > +	int size;
> > +
> > +	if (is_edp(intel_dp)) {
> > +		if (!intel_dp->edid)
> > +			return NULL;
> > +
> > +		size = (intel_dp->edid->extensions + 1) * EDID_LENGTH;
> > +		edid = kmalloc(size, GFP_KERNEL);
> > +		if (!edid)
> > +			return NULL;
> > +
> > +		memcpy(edid, intel_dp->edid, size);
> > +		return edid;
> > +	}
> >  
> > -	ironlake_edp_panel_vdd_on(intel_dp);
> >  	edid = drm_get_edid(connector, adapter);
> > -	ironlake_edp_panel_vdd_off(intel_dp, false);
> >  	return edid;
> >  }
> >  
> > @@ -2134,9 +2149,17 @@ intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *ada
> >  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> >  	int	ret;
> >  
> > -	ironlake_edp_panel_vdd_on(intel_dp);
> > +	if (is_edp(intel_dp)) {
> > +		drm_mode_connector_update_edid_property(connector,
> > +							intel_dp->edid);
> > +		ret = drm_add_edid_modes(connector, intel_dp->edid);
> 
> Hi Jesse, I'm sure you meant to do *something* with that return value. I
> presume it should be equal to intel_dp->edid_mode_count, but is it
> possible it's not? Is it better to return ret or
> intel_dp->edid_mode_count from this function?
> 

Heh there's always something... but yeah it should be equal to the
mode_count; I should either drop the ret= or use it.  Maybe as a
cleanup on top since I'm tired of respinning this one? :p


-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Chris Wilson]

On Fri, 15 Jun 2012 13:52:04 +0300, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 15 Jun 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > They aren't going anywhere, and probing on DDC can cause the panel to
> > blank briefly, so read them up front and cache them for later queries.
> >
> > Jesse's patch revamped. Gotta love those display_info.raw_edid = NULL!
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c    |   48 +++++++++++++++++++++++++++---------
> >  drivers/gpu/drm/i915/intel_drv.h   |    1 +
> >  drivers/gpu/drm/i915/intel_modes.c |   24 ++++++++++++------
> >  3 files changed, 54 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index eb57ec7..207e25f 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -67,6 +67,7 @@ struct intel_dp {
> >  	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
> >  	struct delayed_work panel_vdd_work;
> >  	bool want_panel_vdd;
> > +	struct edid *edid; /* cached for eDP */
> >  };
> >  
> >  /**
> > @@ -2095,26 +2096,50 @@ g4x_dp_detect(struct intel_dp *intel_dp)
> >  }
> >  
> >  static struct edid *
> > -intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> > +_drm_edid_duplicate(struct edid *edid)
> > +{
> > +	struct edid *copy;
> > +	int size;
> > +
> > +	if (edid == NULL)
> > +		return NULL;
> > +
> > +	size = EDID_LENGTH * (1 + edid->extensions);
> > +	*copy = kmalloc(size, GFP_KERNEL);
> 
> You may want to remove that '*' there...

And free the cached edid upon destroy. :)
Mainly seeing if I could convince Jesse that we could do a nice patch
for -fixes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Jani Nikula]

On Fri, 15 Jun 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> They aren't going anywhere, and probing on DDC can cause the panel to
> blank briefly, so read them up front and cache them for later queries.
>
> Jesse's patch revamped. Gotta love those display_info.raw_edid = NULL!
> ---
>  drivers/gpu/drm/i915/intel_dp.c    |   48 +++++++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_drv.h   |    1 +
>  drivers/gpu/drm/i915/intel_modes.c |   24 ++++++++++++------
>  3 files changed, 54 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index eb57ec7..207e25f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -67,6 +67,7 @@ struct intel_dp {
>  	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
>  	struct delayed_work panel_vdd_work;
>  	bool want_panel_vdd;
> +	struct edid *edid; /* cached for eDP */
>  };
>  
>  /**
> @@ -2095,26 +2096,50 @@ g4x_dp_detect(struct intel_dp *intel_dp)
>  }
>  
>  static struct edid *
> -intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> +_drm_edid_duplicate(struct edid *edid)
> +{
> +	struct edid *copy;
> +	int size;
> +
> +	if (edid == NULL)
> +		return NULL;
> +
> +	size = EDID_LENGTH * (1 + edid->extensions);
> +	*copy = kmalloc(size, GFP_KERNEL);

You may want to remove that '*' there...

Jani.

> +	if (copy)
> +		memcpy(copy, edid, size);
> +
> +	return copy;
> +}
> +
> +static struct edid *
> +intel_dp_get_edid(struct drm_connector *connector)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	struct edid	*edid;
> +	struct edid *edid;
> +
> +	if (is_edp(intel_dp))
> +		return _drm_edid_duplicate(intel_dp->edid);
>  
>  	ironlake_edp_panel_vdd_on(intel_dp);
> -	edid = drm_get_edid(connector, adapter);
> +	edid = drm_get_edid(connector, &intel_dp->adapter);
>  	ironlake_edp_panel_vdd_off(intel_dp, false);
> +
>  	return edid;
>  }
>  
>  static int
> -intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *adapter)
> +intel_dp_get_edid_modes(struct drm_connector *connector)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	int	ret;
> +	struct edid *edid;
> +	int ret;
> +
> +	edid = intel_dp_get_edid(intel_dp);
> +	ret = intel_connector_attach_edid(connector, edid);
> +	connector->display_info.raw_edid = NULL;
> +	kfree(edid);
>  
> -	ironlake_edp_panel_vdd_on(intel_dp);
> -	ret = intel_ddc_get_modes(connector, adapter);
> -	ironlake_edp_panel_vdd_off(intel_dp, false);
>  	return ret;
>  }
>  
> @@ -2172,7 +2197,7 @@ static int intel_dp_get_modes(struct drm_connector *connector)
>  	/* We should parse the EDID data and find out if it has an audio sink
>  	 */
>  
> -	ret = intel_dp_get_edid_modes(connector, &intel_dp->adapter);
> +	ret = intel_dp_get_edid_modes(connector);
>  	if (ret) {
>  		if (is_edp(intel_dp) && !intel_dp->panel_fixed_mode) {
>  			struct drm_display_mode *newmode;
> @@ -2485,6 +2510,8 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  			break;
>  	}
>  
> +	intel_dp_i2c_init(intel_dp, intel_connector, name);
> +
>  	/* Cache some DPCD data in the eDP case */
>  	if (is_edp(intel_dp)) {
>  		bool ret;
> @@ -2543,6 +2570,7 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  
>  		ironlake_edp_panel_vdd_on(intel_dp);
>  		ret = intel_dp_get_dpcd(intel_dp);
> +		intel_dp->edid = drm_get_edid(connector, &intel_dp->adapter);
>  		ironlake_edp_panel_vdd_off(intel_dp, false);
>  
>  		if (ret) {
> @@ -2559,8 +2587,6 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  		}
>  	}
>  
> -	intel_dp_i2c_init(intel_dp, intel_connector, name);
> -
>  	intel_encoder->hot_plug = intel_dp_hot_plug;
>  
>  	if (is_edp(intel_dp)) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3e09188..316413d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -336,6 +336,7 @@ struct intel_fbc_work {
>  
>  int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
>  extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus);
> +int intel_connector_attach_edid(struct drm_connector *connector, struct edid *edid);
>  
>  extern void intel_attach_force_audio_property(struct drm_connector *connector);
>  extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
> index d67ec3a..089ed8b 100644
> --- a/drivers/gpu/drm/i915/intel_modes.c
> +++ b/drivers/gpu/drm/i915/intel_modes.c
> @@ -60,6 +60,18 @@ bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus)
>  			    msgs, 2) == 2;
>  }
>  
> +int intel_connector_attach_edid(struct drm_connector *connector,
> +				struct edid *edid)
> +{
> +	int ret;
> +
> +	drm_mode_connector_update_edid_property(connector, edid);
> +	ret = drm_add_edid_modes(connector, edid);
> +	drm_edid_to_eld(connector, edid);
> +
> +	return ret;
> +}
> +
>  /**
>   * intel_ddc_get_modes - get modelist from monitor
>   * @connector: DRM connector device to use
> @@ -71,16 +83,12 @@ int intel_ddc_get_modes(struct drm_connector *connector,
>  			struct i2c_adapter *adapter)
>  {
>  	struct edid *edid;
> -	int ret = 0;
> +	int ret;
>  
>  	edid = drm_get_edid(connector, adapter);
> -	if (edid) {
> -		drm_mode_connector_update_edid_property(connector, edid);
> -		ret = drm_add_edid_modes(connector, edid);
> -		drm_edid_to_eld(connector, edid);
> -		connector->display_info.raw_edid = NULL;
> -		kfree(edid);
> -	}
> +	ret = intel_connector_attach_edid(connector, edid);
> +	connector->display_info.raw_edid = NULL;
> +	kfree(edid);
>  
>  	return ret;
>  }
> -- 
> 1.7.10
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: cache the EDID for eDP panels

[Chris Wilson]

They aren't going anywhere, and probing on DDC can cause the panel to
blank briefly, so read them up front and cache them for later queries.

Jesse's patch revamped. Gotta love those display_info.raw_edid = NULL!
---
 drivers/gpu/drm/i915/intel_dp.c    |   48 +++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h   |    1 +
 drivers/gpu/drm/i915/intel_modes.c |   24 ++++++++++++------
 3 files changed, 54 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index eb57ec7..207e25f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -67,6 +67,7 @@ struct intel_dp {
 	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
+	struct edid *edid; /* cached for eDP */
 };
 
 /**
@@ -2095,26 +2096,50 @@ g4x_dp_detect(struct intel_dp *intel_dp)
 }
 
 static struct edid *
-intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
+_drm_edid_duplicate(struct edid *edid)
+{
+	struct edid *copy;
+	int size;
+
+	if (edid == NULL)
+		return NULL;
+
+	size = EDID_LENGTH * (1 + edid->extensions);
+	*copy = kmalloc(size, GFP_KERNEL);
+	if (copy)
+		memcpy(copy, edid, size);
+
+	return copy;
+}
+
+static struct edid *
+intel_dp_get_edid(struct drm_connector *connector)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct edid	*edid;
+	struct edid *edid;
+
+	if (is_edp(intel_dp))
+		return _drm_edid_duplicate(intel_dp->edid);
 
 	ironlake_edp_panel_vdd_on(intel_dp);
-	edid = drm_get_edid(connector, adapter);
+	edid = drm_get_edid(connector, &intel_dp->adapter);
 	ironlake_edp_panel_vdd_off(intel_dp, false);
+
 	return edid;
 }
 
 static int
-intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *adapter)
+intel_dp_get_edid_modes(struct drm_connector *connector)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	int	ret;
+	struct edid *edid;
+	int ret;
+
+	edid = intel_dp_get_edid(intel_dp);
+	ret = intel_connector_attach_edid(connector, edid);
+	connector->display_info.raw_edid = NULL;
+	kfree(edid);
 
-	ironlake_edp_panel_vdd_on(intel_dp);
-	ret = intel_ddc_get_modes(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return ret;
 }
 
@@ -2172,7 +2197,7 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 	/* We should parse the EDID data and find out if it has an audio sink
 	 */
 
-	ret = intel_dp_get_edid_modes(connector, &intel_dp->adapter);
+	ret = intel_dp_get_edid_modes(connector);
 	if (ret) {
 		if (is_edp(intel_dp) && !intel_dp->panel_fixed_mode) {
 			struct drm_display_mode *newmode;
@@ -2485,6 +2510,8 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			break;
 	}
 
+	intel_dp_i2c_init(intel_dp, intel_connector, name);
+
 	/* Cache some DPCD data in the eDP case */
 	if (is_edp(intel_dp)) {
 		bool ret;
@@ -2543,6 +2570,7 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 
 		ironlake_edp_panel_vdd_on(intel_dp);
 		ret = intel_dp_get_dpcd(intel_dp);
+		intel_dp->edid = drm_get_edid(connector, &intel_dp->adapter);
 		ironlake_edp_panel_vdd_off(intel_dp, false);
 
 		if (ret) {
@@ -2559,8 +2587,6 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 		}
 	}
 
-	intel_dp_i2c_init(intel_dp, intel_connector, name);
-
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
 	if (is_edp(intel_dp)) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3e09188..316413d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -336,6 +336,7 @@ struct intel_fbc_work {
 
 int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
 extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus);
+int intel_connector_attach_edid(struct drm_connector *connector, struct edid *edid);
 
 extern void intel_attach_force_audio_property(struct drm_connector *connector);
 extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
index d67ec3a..089ed8b 100644
--- a/drivers/gpu/drm/i915/intel_modes.c
+++ b/drivers/gpu/drm/i915/intel_modes.c
@@ -60,6 +60,18 @@ bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus)
 			    msgs, 2) == 2;
 }
 
+int intel_connector_attach_edid(struct drm_connector *connector,
+				struct edid *edid)
+{
+	int ret;
+
+	drm_mode_connector_update_edid_property(connector, edid);
+	ret = drm_add_edid_modes(connector, edid);
+	drm_edid_to_eld(connector, edid);
+
+	return ret;
+}
+
 /**
  * intel_ddc_get_modes - get modelist from monitor
  * @connector: DRM connector device to use
@@ -71,16 +83,12 @@ int intel_ddc_get_modes(struct drm_connector *connector,
 			struct i2c_adapter *adapter)
 {
 	struct edid *edid;
-	int ret = 0;
+	int ret;
 
 	edid = drm_get_edid(connector, adapter);
-	if (edid) {
-		drm_mode_connector_update_edid_property(connector, edid);
-		ret = drm_add_edid_modes(connector, edid);
-		drm_edid_to_eld(connector, edid);
-		connector->display_info.raw_edid = NULL;
-		kfree(edid);
-	}
+	ret = intel_connector_attach_edid(connector, edid);
+	connector->display_info.raw_edid = NULL;
+	kfree(edid);
 
 	return ret;
 }
-- 
1.7.10

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Jani Nikula]

On Thu, 14 Jun 2012, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> They aren't going anywhere, and probing on DDC can cause the panel to
> blank briefly, so read them up front and cache them for later queries.
>
> v2: fix potential NULL derefs in intel_dp_get_edid_modes and
>     intel_dp_get_edid (Jani)
>     copy full EDID length, including extension blocks (Takashi)
>     free EDID on teardown (Takashi)
> v3: malloc a new EDID buffer that's big enough for the memcpy (Chris)
> v4: change handling of NULL EDIDs, just preserve the NULL behavior
>     across detects and mode list fetches rather than trying to re-fetch
>     the EDID (Chris)
> v5: be glad that Chris is around to remind me to hit C-x C-s before
>     committing.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=46856
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   49 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6538c46..69d2f0c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -32,6 +32,7 @@
>  #include "drm.h"
>  #include "drm_crtc.h"
>  #include "drm_crtc_helper.h"
> +#include "drm_edid.h"
>  #include "intel_drv.h"
>  #include "i915_drm.h"
>  #include "i915_drv.h"
> @@ -67,6 +68,8 @@ struct intel_dp {
>  	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
>  	struct delayed_work panel_vdd_work;
>  	bool want_panel_vdd;
> +	struct edid *edid; /* cached EDID for eDP */
> +	int edid_mode_count;
>  };
>  
>  /**
> @@ -2121,10 +2124,22 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct edid	*edid;
> +	int size;
> +
> +	if (is_edp(intel_dp)) {
> +		if (!intel_dp->edid)
> +			return NULL;
> +
> +		size = (intel_dp->edid->extensions + 1) * EDID_LENGTH;
> +		edid = kmalloc(size, GFP_KERNEL);
> +		if (!edid)
> +			return NULL;
> +
> +		memcpy(edid, intel_dp->edid, size);
> +		return edid;
> +	}
>  
> -	ironlake_edp_panel_vdd_on(intel_dp);
>  	edid = drm_get_edid(connector, adapter);
> -	ironlake_edp_panel_vdd_off(intel_dp, false);
>  	return edid;
>  }
>  
> @@ -2134,9 +2149,17 @@ intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *ada
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	int	ret;
>  
> -	ironlake_edp_panel_vdd_on(intel_dp);
> +	if (is_edp(intel_dp)) {
> +		drm_mode_connector_update_edid_property(connector,
> +							intel_dp->edid);
> +		ret = drm_add_edid_modes(connector, intel_dp->edid);

Hi Jesse, I'm sure you meant to do *something* with that return value. I
presume it should be equal to intel_dp->edid_mode_count, but is it
possible it's not? Is it better to return ret or
intel_dp->edid_mode_count from this function?

BR,
Jani.


> +		drm_edid_to_eld(connector,
> +				intel_dp->edid);
> +		connector->display_info.raw_edid = NULL;
> +		return intel_dp->edid_mode_count;
> +	}
> +
>  	ret = intel_ddc_get_modes(connector, adapter);
> -	ironlake_edp_panel_vdd_off(intel_dp, false);
>  	return ret;
>  }
>  
> @@ -2326,6 +2349,7 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  	i2c_del_adapter(&intel_dp->adapter);
>  	drm_encoder_cleanup(encoder);
>  	if (is_edp(intel_dp)) {
> +		kfree(intel_dp->edid);
>  		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
>  		ironlake_panel_vdd_off_sync(intel_dp);
>  	}
> @@ -2509,11 +2533,14 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  			break;
>  	}
>  
> +	intel_dp_i2c_init(intel_dp, intel_connector, name);
> +
>  	/* Cache some DPCD data in the eDP case */
>  	if (is_edp(intel_dp)) {
>  		bool ret;
>  		struct edp_power_seq	cur, vbt;
>  		u32 pp_on, pp_off, pp_div;
> +		struct edid *edid;
>  
>  		pp_on = I915_READ(PCH_PP_ON_DELAYS);
>  		pp_off = I915_READ(PCH_PP_OFF_DELAYS);
> @@ -2581,9 +2608,19 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  			intel_dp_destroy(&intel_connector->base);
>  			return;
>  		}
> -	}
>  
> -	intel_dp_i2c_init(intel_dp, intel_connector, name);
> +		ironlake_edp_panel_vdd_on(intel_dp);
> +		edid = drm_get_edid(connector, &intel_dp->adapter);
> +		if (edid) {
> +			drm_mode_connector_update_edid_property(connector,
> +								edid);
> +			intel_dp->edid_mode_count =
> +				drm_add_edid_modes(connector, edid);
> +			drm_edid_to_eld(connector, edid);
> +			intel_dp->edid = edid;
> +		}
> +		ironlake_edp_panel_vdd_off(intel_dp, false);
> +	}
>  
>  	intel_encoder->hot_plug = intel_dp_hot_plug;
>  
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Jesse Barnes]

On Thu, 14 Jun 2012 20:42:15 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 14 Jun 2012 15:28:33 -0400, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > They aren't going anywhere, and probing on DDC can cause the panel to
> > blank briefly, so read them up front and cache them for later queries.
> > 
> > v2: fix potential NULL derefs in intel_dp_get_edid_modes and
> >     intel_dp_get_edid (Jani)
> >     copy full EDID length, including extension blocks (Takashi)
> >     free EDID on teardown (Takashi)
> > v3: malloc a new EDID buffer that's big enough for the memcpy (Chris)
> > v4: change handling of NULL EDIDs, just preserve the NULL behavior
> >     across detects and mode list fetches rather than trying to re-fetch
> >     the EDID (Chris)
> > v5: be glad that Chris is around to remind me to hit C-x C-s before
> >     committing.
> 
> I do 'git add' reminders as well; by appointment only.
> 
> And now to really pour rain on your fire. Why are we repeating quite
> obscure logic from intel_modes.c? Can we not export a function to attach
> an edid to a connector and then refactor intel_dp.c to only have a
> single edid reader where you could add your magic eDP EDID cache
> handling in single place.
> 
> So intel_dp_get_edid_modes calls intel_dp_get_edid() and
> intel_connector_attach_edid() rather the single-shot
> intel_ddc_get_modes().

I totally agree this can be more generic.  However, Daniel wanted
something for -fixes that's fairly self-contained.  I think we should
push this for the current kernel (and potentially stable) since it
fixes a bug, and your stuff or Daniel's stuff can go into -next that
makes every connector better.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Chris Wilson]

On Thu, 14 Jun 2012 15:28:33 -0400, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> They aren't going anywhere, and probing on DDC can cause the panel to
> blank briefly, so read them up front and cache them for later queries.
> 
> v2: fix potential NULL derefs in intel_dp_get_edid_modes and
>     intel_dp_get_edid (Jani)
>     copy full EDID length, including extension blocks (Takashi)
>     free EDID on teardown (Takashi)
> v3: malloc a new EDID buffer that's big enough for the memcpy (Chris)
> v4: change handling of NULL EDIDs, just preserve the NULL behavior
>     across detects and mode list fetches rather than trying to re-fetch
>     the EDID (Chris)
> v5: be glad that Chris is around to remind me to hit C-x C-s before
>     committing.

I do 'git add' reminders as well; by appointment only.

And now to really pour rain on your fire. Why are we repeating quite
obscure logic from intel_modes.c? Can we not export a function to attach
an edid to a connector and then refactor intel_dp.c to only have a
single edid reader where you could add your magic eDP EDID cache
handling in single place.

So intel_dp_get_edid_modes calls intel_dp_get_edid() and
intel_connector_attach_edid() rather the single-shot
intel_ddc_get_modes().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: cache the EDID for eDP panels

[Jesse Barnes]

They aren't going anywhere, and probing on DDC can cause the panel to
blank briefly, so read them up front and cache them for later queries.

v2: fix potential NULL derefs in intel_dp_get_edid_modes and
    intel_dp_get_edid (Jani)
    copy full EDID length, including extension blocks (Takashi)
    free EDID on teardown (Takashi)
v3: malloc a new EDID buffer that's big enough for the memcpy (Chris)
v4: change handling of NULL EDIDs, just preserve the NULL behavior
    across detects and mode list fetches rather than trying to re-fetch
    the EDID (Chris)
v5: be glad that Chris is around to remind me to hit C-x C-s before
    committing.

References: https://bugs.freedesktop.org/show_bug.cgi?id=46856
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   49 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6538c46..69d2f0c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -32,6 +32,7 @@
 #include "drm.h"
 #include "drm_crtc.h"
 #include "drm_crtc_helper.h"
+#include "drm_edid.h"
 #include "intel_drv.h"
 #include "i915_drm.h"
 #include "i915_drv.h"
@@ -67,6 +68,8 @@ struct intel_dp {
 	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
+	struct edid *edid; /* cached EDID for eDP */
+	int edid_mode_count;
 };
 
 /**
@@ -2121,10 +2124,22 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct edid	*edid;
+	int size;
+
+	if (is_edp(intel_dp)) {
+		if (!intel_dp->edid)
+			return NULL;
+
+		size = (intel_dp->edid->extensions + 1) * EDID_LENGTH;
+		edid = kmalloc(size, GFP_KERNEL);
+		if (!edid)
+			return NULL;
+
+		memcpy(edid, intel_dp->edid, size);
+		return edid;
+	}
 
-	ironlake_edp_panel_vdd_on(intel_dp);
 	edid = drm_get_edid(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return edid;
 }
 
@@ -2134,9 +2149,17 @@ intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *ada
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	int	ret;
 
-	ironlake_edp_panel_vdd_on(intel_dp);
+	if (is_edp(intel_dp)) {
+		drm_mode_connector_update_edid_property(connector,
+							intel_dp->edid);
+		ret = drm_add_edid_modes(connector, intel_dp->edid);
+		drm_edid_to_eld(connector,
+				intel_dp->edid);
+		connector->display_info.raw_edid = NULL;
+		return intel_dp->edid_mode_count;
+	}
+
 	ret = intel_ddc_get_modes(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return ret;
 }
 
@@ -2326,6 +2349,7 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	i2c_del_adapter(&intel_dp->adapter);
 	drm_encoder_cleanup(encoder);
 	if (is_edp(intel_dp)) {
+		kfree(intel_dp->edid);
 		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
 		ironlake_panel_vdd_off_sync(intel_dp);
 	}
@@ -2509,11 +2533,14 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			break;
 	}
 
+	intel_dp_i2c_init(intel_dp, intel_connector, name);
+
 	/* Cache some DPCD data in the eDP case */
 	if (is_edp(intel_dp)) {
 		bool ret;
 		struct edp_power_seq	cur, vbt;
 		u32 pp_on, pp_off, pp_div;
+		struct edid *edid;
 
 		pp_on = I915_READ(PCH_PP_ON_DELAYS);
 		pp_off = I915_READ(PCH_PP_OFF_DELAYS);
@@ -2581,9 +2608,19 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			intel_dp_destroy(&intel_connector->base);
 			return;
 		}
-	}
 
-	intel_dp_i2c_init(intel_dp, intel_connector, name);
+		ironlake_edp_panel_vdd_on(intel_dp);
+		edid = drm_get_edid(connector, &intel_dp->adapter);
+		if (edid) {
+			drm_mode_connector_update_edid_property(connector,
+								edid);
+			intel_dp->edid_mode_count =
+				drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			intel_dp->edid = edid;
+		}
+		ironlake_edp_panel_vdd_off(intel_dp, false);
+	}
 
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
-- 
1.7.9.5

[PATCH] drm/i915: cache the EDID for eDP panels

[Jesse Barnes]

They aren't going anywhere, and probing on DDC can cause the panel to
blank briefly, so read them up front and cache them for later queries.

v2: fix potential NULL derefs in intel_dp_get_edid_modes and
    intel_dp_get_edid (Jani)
    copy full EDID length, including extension blocks (Takashi)
    free EDID on teardown (Takashi)
v3: malloc a new EDID buffer that's big enough for the memcpy (Chris)
v4: change handling of NULL EDIDs, just preserve the NULL behavior
    across detects and mode list fetches rather than trying to re-fetch
    the EDID (Chris)

References: https://bugs.freedesktop.org/show_bug.cgi?id=46856
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   49 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6538c46..8cba3c3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -32,6 +32,7 @@
 #include "drm.h"
 #include "drm_crtc.h"
 #include "drm_crtc_helper.h"
+#include "drm_edid.h"
 #include "intel_drv.h"
 #include "i915_drm.h"
 #include "i915_drv.h"
@@ -67,6 +68,8 @@ struct intel_dp {
 	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
+	struct edid *edid; /* cached EDID for eDP */
+	int edid_mode_count;
 };
 
 /**
@@ -2121,10 +2124,22 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct edid	*edid;
+	int size;
+
+	if (is_edp(intel_dp)) {
+		if (!intel_dp->edid)
+			return NULL;
+
+		size = (intel_dp->edid->extensions + 1) * EDID_LENGTH;
+		edid = kmalloc(size, GFP_KERNEL);
+		if (!edid)
+			return NULL;
+
+		memcpy(edid, intel_dp->edid, size);
+		return edid;
+	}
 
-	ironlake_edp_panel_vdd_on(intel_dp);
 	edid = drm_get_edid(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return edid;
 }
 
@@ -2134,9 +2149,17 @@ intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *ada
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	int	ret;
 
-	ironlake_edp_panel_vdd_on(intel_dp);
+	if (is_edp(intel_dp) && intel_dp->edid) {
+		drm_mode_connector_update_edid_property(connector,
+							intel_dp->edid);
+		ret = drm_add_edid_modes(connector, intel_dp->edid);
+		drm_edid_to_eld(connector,
+				intel_dp->edid);
+		connector->display_info.raw_edid = NULL;
+		return intel_dp->edid_mode_count;
+	}
+
 	ret = intel_ddc_get_modes(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return ret;
 }
 
@@ -2326,6 +2349,7 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	i2c_del_adapter(&intel_dp->adapter);
 	drm_encoder_cleanup(encoder);
 	if (is_edp(intel_dp)) {
+		kfree(intel_dp->edid);
 		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
 		ironlake_panel_vdd_off_sync(intel_dp);
 	}
@@ -2509,11 +2533,14 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			break;
 	}
 
+	intel_dp_i2c_init(intel_dp, intel_connector, name);
+
 	/* Cache some DPCD data in the eDP case */
 	if (is_edp(intel_dp)) {
 		bool ret;
 		struct edp_power_seq	cur, vbt;
 		u32 pp_on, pp_off, pp_div;
+		struct edid *edid;
 
 		pp_on = I915_READ(PCH_PP_ON_DELAYS);
 		pp_off = I915_READ(PCH_PP_OFF_DELAYS);
@@ -2581,9 +2608,19 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			intel_dp_destroy(&intel_connector->base);
 			return;
 		}
-	}
 
-	intel_dp_i2c_init(intel_dp, intel_connector, name);
+		ironlake_edp_panel_vdd_on(intel_dp);
+		edid = drm_get_edid(connector, &intel_dp->adapter);
+		if (edid) {
+			drm_mode_connector_update_edid_property(connector,
+								edid);
+			intel_dp->edid_mode_count =
+				drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			intel_dp->edid = edid;
+		}
+		ironlake_edp_panel_vdd_off(intel_dp, false);
+	}
 
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
-- 
1.7.9.5

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Jesse Barnes]

On Thu, 14 Jun 2012 19:37:32 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 14 Jun 2012 14:30:52 -0400, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > They aren't going anywhere, and probing on DDC can cause the panel to
> > blank briefly, so read them up front and cache them for later queries.
> > 
> > v2: fix potential NULL derefs in intel_dp_get_edid_modes and
> >     intel_dp_get_edid (Jani)
> >     copy full EDID length, including extension blocks (Takashi)
> >     free EDID on teardown (Takashi)
> > v3: malloc a new EDID buffer that's big enough for the memcpy (Chris)
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=46856
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> > @@ -2121,10 +2124,19 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> >  {
> >  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> >  	struct edid	*edid;
> > +	int size;
> > +
> > +	if (is_edp(intel_dp) && intel_dp->edid) {
> > +		size = (intel_dp->edid->extensions + 1) * EDID_LENGTH;
> > +		edid = kmalloc(size, GFP_KERNEL);
> > +		if (!edid)
> > +			return NULL;
> > +
> > +		memcpy(edid, intel_dp->edid, size);
> > +		return edid;
> > +	}
> >  
> > -	ironlake_edp_panel_vdd_on(intel_dp);
> >  	edid = drm_get_edid(connector, adapter);
> > -	ironlake_edp_panel_vdd_off(intel_dp, false);
> >  	return edid;
> 
> This makes me a little uneasy, as the wording here makes it sound like
> it is possible for us to initiate a DP aux xfer without raising VDD on
> the eDP panel. Having made the decision to retreive the EDID for a panel
> only once, we should use that result for all future queries i.e. if it
> was NULL during init, assume it will be NULL now as well. (I hope nobody
> brings out an eDP-to-HDMI dongle!)

Oh good call.  Now that I'm checking for whether we have a cached edid
I should leave the aux toggling in...

Thanks,
Jesse

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Chris Wilson]

On Thu, 14 Jun 2012 14:30:52 -0400, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> They aren't going anywhere, and probing on DDC can cause the panel to
> blank briefly, so read them up front and cache them for later queries.
> 
> v2: fix potential NULL derefs in intel_dp_get_edid_modes and
>     intel_dp_get_edid (Jani)
>     copy full EDID length, including extension blocks (Takashi)
>     free EDID on teardown (Takashi)
> v3: malloc a new EDID buffer that's big enough for the memcpy (Chris)
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=46856
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
> @@ -2121,10 +2124,19 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  {
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct edid	*edid;
> +	int size;
> +
> +	if (is_edp(intel_dp) && intel_dp->edid) {
> +		size = (intel_dp->edid->extensions + 1) * EDID_LENGTH;
> +		edid = kmalloc(size, GFP_KERNEL);
> +		if (!edid)
> +			return NULL;
> +
> +		memcpy(edid, intel_dp->edid, size);
> +		return edid;
> +	}
>  
> -	ironlake_edp_panel_vdd_on(intel_dp);
>  	edid = drm_get_edid(connector, adapter);
> -	ironlake_edp_panel_vdd_off(intel_dp, false);
>  	return edid;

This makes me a little uneasy, as the wording here makes it sound like
it is possible for us to initiate a DP aux xfer without raising VDD on
the eDP panel. Having made the decision to retreive the EDID for a panel
only once, we should use that result for all future queries i.e. if it
was NULL during init, assume it will be NULL now as well. (I hope nobody
brings out an eDP-to-HDMI dongle!)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: cache the EDID for eDP panels

[Jesse Barnes]

They aren't going anywhere, and probing on DDC can cause the panel to
blank briefly, so read them up front and cache them for later queries.

v2: fix potential NULL derefs in intel_dp_get_edid_modes and
    intel_dp_get_edid (Jani)
    copy full EDID length, including extension blocks (Takashi)
    free EDID on teardown (Takashi)
v3: malloc a new EDID buffer that's big enough for the memcpy (Chris)

References: https://bugs.freedesktop.org/show_bug.cgi?id=46856
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   46 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6538c46..46428d9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -32,6 +32,7 @@
 #include "drm.h"
 #include "drm_crtc.h"
 #include "drm_crtc_helper.h"
+#include "drm_edid.h"
 #include "intel_drv.h"
 #include "i915_drm.h"
 #include "i915_drv.h"
@@ -67,6 +68,8 @@ struct intel_dp {
 	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
+	struct edid *edid; /* cached EDID for eDP */
+	int edid_mode_count;
 };
 
 /**
@@ -2121,10 +2124,19 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct edid	*edid;
+	int size;
+
+	if (is_edp(intel_dp) && intel_dp->edid) {
+		size = (intel_dp->edid->extensions + 1) * EDID_LENGTH;
+		edid = kmalloc(size, GFP_KERNEL);
+		if (!edid)
+			return NULL;
+
+		memcpy(edid, intel_dp->edid, size);
+		return edid;
+	}
 
-	ironlake_edp_panel_vdd_on(intel_dp);
 	edid = drm_get_edid(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return edid;
 }
 
@@ -2134,9 +2146,17 @@ intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *ada
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	int	ret;
 
-	ironlake_edp_panel_vdd_on(intel_dp);
+	if (is_edp(intel_dp) && intel_dp->edid) {
+		drm_mode_connector_update_edid_property(connector,
+							intel_dp->edid);
+		ret = drm_add_edid_modes(connector, intel_dp->edid);
+		drm_edid_to_eld(connector,
+				intel_dp->edid);
+		connector->display_info.raw_edid = NULL;
+		return intel_dp->edid_mode_count;
+	}
+
 	ret = intel_ddc_get_modes(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return ret;
 }
 
@@ -2326,6 +2346,7 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	i2c_del_adapter(&intel_dp->adapter);
 	drm_encoder_cleanup(encoder);
 	if (is_edp(intel_dp)) {
+		kfree(intel_dp->edid);
 		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
 		ironlake_panel_vdd_off_sync(intel_dp);
 	}
@@ -2509,11 +2530,14 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			break;
 	}
 
+	intel_dp_i2c_init(intel_dp, intel_connector, name);
+
 	/* Cache some DPCD data in the eDP case */
 	if (is_edp(intel_dp)) {
 		bool ret;
 		struct edp_power_seq	cur, vbt;
 		u32 pp_on, pp_off, pp_div;
+		struct edid *edid;
 
 		pp_on = I915_READ(PCH_PP_ON_DELAYS);
 		pp_off = I915_READ(PCH_PP_OFF_DELAYS);
@@ -2581,9 +2605,19 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			intel_dp_destroy(&intel_connector->base);
 			return;
 		}
-	}
 
-	intel_dp_i2c_init(intel_dp, intel_connector, name);
+		ironlake_edp_panel_vdd_on(intel_dp);
+		edid = drm_get_edid(connector, &intel_dp->adapter);
+		if (edid) {
+			drm_mode_connector_update_edid_property(connector,
+								edid);
+			intel_dp->edid_mode_count =
+				drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			intel_dp->edid = edid;
+		}
+		ironlake_edp_panel_vdd_off(intel_dp, false);
+	}
 
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
-- 
1.7.9.5

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Jesse Barnes]

On Wed, 06 Jun 2012 09:36:44 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> At Tue,  5 Jun 2012 16:54:27 -0400,
> Jesse Barnes wrote:
> > 
> > They aren't going anywhere, and probing on DDC can cause the panel to
> > blank briefly, so read them up front and cache them for later queries.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Ok I think I fixed the issues you and Jani reported, thanks for the
review.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Takashi Iwai]

At Tue,  5 Jun 2012 16:54:27 -0400,
Jesse Barnes wrote:
> 
> They aren't going anywhere, and probing on DDC can cause the panel to
> blank briefly, so read them up front and cache them for later queries.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   43 +++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9b2effc..e47ae37 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -32,6 +32,7 @@
>  #include "drm.h"
>  #include "drm_crtc.h"
>  #include "drm_crtc_helper.h"
> +#include "drm_edid.h"
>  #include "intel_drv.h"
>  #include "i915_drm.h"
>  #include "i915_drv.h"
> @@ -67,6 +68,8 @@ struct intel_dp {
>  	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
>  	struct delayed_work panel_vdd_work;
>  	bool want_panel_vdd;
> +	struct edid *edid; /* cached EDID for eDP */
> +	int edid_mode_count;
>  };
>  
>  /**
> @@ -2094,9 +2097,16 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct edid	*edid;
>  
> -	ironlake_edp_panel_vdd_on(intel_dp);
> +	if (is_edp(intel_dp)) {
> +		edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
> +		if (!edid)
> +			return NULL;
> +
> +		memcpy(edid, intel_dp->edid, EDID_LENGTH);
> +		return edid;

It might be bigger than EDID_LENGTH when it contains extensions.
Better to create a helper function like drm_copy_edid()?

Also, there is no kfree() corresponding to intel_dp->edid, so a small
memory leak when unloading the module.  (Ditto for intel_lvds.c, BTW)


Takashi

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Jani Nikula]

Hi Jesse, please find a couple of comments below.

BR,
Jani.

On Tue, 05 Jun 2012, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> They aren't going anywhere, and probing on DDC can cause the panel to
> blank briefly, so read them up front and cache them for later queries.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   43 +++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9b2effc..e47ae37 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -32,6 +32,7 @@
>  #include "drm.h"
>  #include "drm_crtc.h"
>  #include "drm_crtc_helper.h"
> +#include "drm_edid.h"
>  #include "intel_drv.h"
>  #include "i915_drm.h"
>  #include "i915_drv.h"
> @@ -67,6 +68,8 @@ struct intel_dp {
>  	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
>  	struct delayed_work panel_vdd_work;
>  	bool want_panel_vdd;
> +	struct edid *edid; /* cached EDID for eDP */
> +	int edid_mode_count;
>  };
>  
>  /**
> @@ -2094,9 +2097,16 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct edid	*edid;
>  
> -	ironlake_edp_panel_vdd_on(intel_dp);
> +	if (is_edp(intel_dp)) {
> +		edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
> +		if (!edid)
> +			return NULL;
> +
> +		memcpy(edid, intel_dp->edid, EDID_LENGTH);

I suppose it may be justified to use EDID_LENGTH rather than
sizeof(*edid) here, but would it be too paranoid to stick this
somewhere?

	BUILD_BUG_ON(sizeof(struct edid) != EDID_LENGTH);

> +		return edid;
> +	}
> +
>  	edid = drm_get_edid(connector, adapter);
> -	ironlake_edp_panel_vdd_off(intel_dp, false);
>  	return edid;
>  }
>  
> @@ -2106,9 +2116,17 @@ intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *ada
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	int	ret;
>  
> -	ironlake_edp_panel_vdd_on(intel_dp);
> +	if (is_edp(intel_dp)) {
> +		drm_mode_connector_update_edid_property(connector,
> +							intel_dp->edid);
> +		ret = drm_add_edid_modes(connector, intel_dp->edid);
> +		drm_edid_to_eld(connector,
> +				intel_dp->edid);
> +		connector->display_info.raw_edid = NULL;
> +		return intel_dp->edid_mode_count;
> +	}
> +
>  	ret = intel_ddc_get_modes(connector, adapter);
> -	ironlake_edp_panel_vdd_off(intel_dp, false);
>  	return ret;
>  }
>  
> @@ -2479,11 +2497,14 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  			break;
>  	}
>  
> +	intel_dp_i2c_init(intel_dp, intel_connector, name);
> +
>  	/* Cache some DPCD data in the eDP case */
>  	if (is_edp(intel_dp)) {
>  		bool ret;
>  		struct edp_power_seq	cur, vbt;
>  		u32 pp_on, pp_off, pp_div;
> +		struct edid *edid;
>  
>  		pp_on = I915_READ(PCH_PP_ON_DELAYS);
>  		pp_off = I915_READ(PCH_PP_OFF_DELAYS);
> @@ -2551,9 +2572,19 @@ intel_dp_init(struct drm_device *dev, int output_reg)
>  			intel_dp_destroy(&intel_connector->base);
>  			return;
>  		}
> -	}
>  
> -	intel_dp_i2c_init(intel_dp, intel_connector, name);
> +		ironlake_edp_panel_vdd_on(intel_dp);
> +		edid = drm_get_edid(connector, &intel_dp->adapter);
> +		if (edid) {

This if condition makes me believe it's possible edid == NULL, but if
that happens, I think the additions to intel_dp_get_edid() and
intel_dp_get_edid_modes() would oops.

> +			drm_mode_connector_update_edid_property(connector,
> +								edid);
> +			intel_dp->edid_mode_count =
> +				drm_add_edid_modes(connector, edid);
> +			drm_edid_to_eld(connector, edid);
> +			intel_dp->edid = edid;
> +		}
> +		ironlake_edp_panel_vdd_off(intel_dp, false);
> +	}
>  
>  	intel_encoder->hot_plug = intel_dp_hot_plug;
>  
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Daniel Vetter]

On Tue, Jun 05, 2012 at 02:41:15PM -0700, Jesse Barnes wrote:
> On Tue, 05 Jun 2012 22:38:02 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Tue, 5 Jun 2012 14:26:11 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > Yeah that makes sense.  Not sure how much this conflicts with Daniel's
> > > hot plug rework either; there's likely some overlap.
> > 
> > We could have made that Daniel's problem.... ;)
> > http://lists.freedesktop.org/archives/intel-gfx/2011-April/010401.html
> 
> Hey look I even reviewed it.
> 
> Pick up the pieces, Daniel! :)

I agree looks nice. Can I ask for a resend? Afaics all the stuff got
acks/r-bs safe for patch 6 (maybe just drop that one). And I agree with
Jesse's bikeshed on patch 8, is_panel sounds more like what we want.

Dunno about patch 10, killing that pointless flicker would certainly be
nice. I guess we could just merge it and see what happens, then claim
innonce when getting caught with our hands in the cookie jar ;-)

Oh, and I don't think there's much overlap with the hpd stuff of mine,
that only caches edids at ->detect time and relies on core drm changes to
make calls to ->detect happen less often.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Jesse Barnes]

On Tue, 05 Jun 2012 22:38:02 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue, 5 Jun 2012 14:26:11 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Yeah that makes sense.  Not sure how much this conflicts with Daniel's
> > hot plug rework either; there's likely some overlap.
> 
> We could have made that Daniel's problem.... ;)
> http://lists.freedesktop.org/archives/intel-gfx/2011-April/010401.html

Hey look I even reviewed it.

Pick up the pieces, Daniel! :)

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Chris Wilson]

On Tue, 5 Jun 2012 14:26:11 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Yeah that makes sense.  Not sure how much this conflicts with Daniel's
> hot plug rework either; there's likely some overlap.

We could have made that Daniel's problem.... ;)
http://lists.freedesktop.org/archives/intel-gfx/2011-April/010401.html
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Jesse Barnes]

On Tue, 05 Jun 2012 22:02:25 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue,  5 Jun 2012 16:54:27 -0400, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > They aren't going anywhere, and probing on DDC can cause the panel to
> > blank briefly, so read them up front and cache them for later queries.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Maybe we could harmonise more of the common panel tasks between LVDS and
> eDP, and help fight code duplication?

Yeah that makes sense.  Not sure how much this conflicts with Daniel's
hot plug rework either; there's likely some overlap.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: cache the EDID for eDP panels

[Chris Wilson]

On Tue,  5 Jun 2012 16:54:27 -0400, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> They aren't going anywhere, and probing on DDC can cause the panel to
> blank briefly, so read them up front and cache them for later queries.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Maybe we could harmonise more of the common panel tasks between LVDS and
eDP, and help fight code duplication?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: cache the EDID for eDP panels

[Jesse Barnes]

They aren't going anywhere, and probing on DDC can cause the panel to
blank briefly, so read them up front and cache them for later queries.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   43 +++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9b2effc..e47ae37 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -32,6 +32,7 @@
 #include "drm.h"
 #include "drm_crtc.h"
 #include "drm_crtc_helper.h"
+#include "drm_edid.h"
 #include "intel_drv.h"
 #include "i915_drm.h"
 #include "i915_drv.h"
@@ -67,6 +68,8 @@ struct intel_dp {
 	struct drm_display_mode *panel_fixed_mode;  /* for eDP */
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
+	struct edid *edid; /* cached EDID for eDP */
+	int edid_mode_count;
 };
 
 /**
@@ -2094,9 +2097,16 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct edid	*edid;
 
-	ironlake_edp_panel_vdd_on(intel_dp);
+	if (is_edp(intel_dp)) {
+		edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
+		if (!edid)
+			return NULL;
+
+		memcpy(edid, intel_dp->edid, EDID_LENGTH);
+		return edid;
+	}
+
 	edid = drm_get_edid(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return edid;
 }
 
@@ -2106,9 +2116,17 @@ intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *ada
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	int	ret;
 
-	ironlake_edp_panel_vdd_on(intel_dp);
+	if (is_edp(intel_dp)) {
+		drm_mode_connector_update_edid_property(connector,
+							intel_dp->edid);
+		ret = drm_add_edid_modes(connector, intel_dp->edid);
+		drm_edid_to_eld(connector,
+				intel_dp->edid);
+		connector->display_info.raw_edid = NULL;
+		return intel_dp->edid_mode_count;
+	}
+
 	ret = intel_ddc_get_modes(connector, adapter);
-	ironlake_edp_panel_vdd_off(intel_dp, false);
 	return ret;
 }
 
@@ -2479,11 +2497,14 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			break;
 	}
 
+	intel_dp_i2c_init(intel_dp, intel_connector, name);
+
 	/* Cache some DPCD data in the eDP case */
 	if (is_edp(intel_dp)) {
 		bool ret;
 		struct edp_power_seq	cur, vbt;
 		u32 pp_on, pp_off, pp_div;
+		struct edid *edid;
 
 		pp_on = I915_READ(PCH_PP_ON_DELAYS);
 		pp_off = I915_READ(PCH_PP_OFF_DELAYS);
@@ -2551,9 +2572,19 @@ intel_dp_init(struct drm_device *dev, int output_reg)
 			intel_dp_destroy(&intel_connector->base);
 			return;
 		}
-	}
 
-	intel_dp_i2c_init(intel_dp, intel_connector, name);
+		ironlake_edp_panel_vdd_on(intel_dp);
+		edid = drm_get_edid(connector, &intel_dp->adapter);
+		if (edid) {
+			drm_mode_connector_update_edid_property(connector,
+								edid);
+			intel_dp->edid_mode_count =
+				drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			intel_dp->edid = edid;
+		}
+		ironlake_edp_panel_vdd_off(intel_dp, false);
+	}
 
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
-- 
1.7.9.5