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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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