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

[Jani Nikula <jani.nikula@linux.intel.com>]

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