From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH] drm/i915: cache the EDID for eDP panels Date: Fri, 15 Jun 2012 10:31:06 -0700 Message-ID: <20120615103106.730ab802@jbarnes-desktop> References: <1339702113-2961-1-git-send-email-jbarnes@virtuousgeek.org> <87wr39ni4h.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from oproxy7-pub.bluehost.com (oproxy7-pub.bluehost.com [67.222.55.9]) by gabe.freedesktop.org (Postfix) with SMTP id A894A9E768 for ; Fri, 15 Jun 2012 10:31:07 -0700 (PDT) In-Reply-To: <87wr39ni4h.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Jani Nikula Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, 15 Jun 2012 10:34:22 +0300 Jani Nikula wrote: > On Thu, 14 Jun 2012, 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. > > > > 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 > > --- > > 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