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

Jesse Barnes
2012-06-15T17:31:07Z

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