From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 1/2] drm/i915/dp: Cache EDID for a detection cycle Date: Mon, 1 Sep 2014 15:05:26 +0300 Message-ID: <20140901120526.GD4193@intel.com> References: <1407832564-21722-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id A09D289C94 for ; Mon, 1 Sep 2014 05:06:37 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1407832564-21722-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Aug 12, 2014 at 09:36:03AM +0100, Chris Wilson wrote: > As we may query the edid multiple times following a detect, record the > EDID found during output discovery and reuse it. This is a separate > issue from caching the output EDID across detection cycles. My only real concern here is what happens when someone forces the connector to connected. Given you only populate detect_edid in ->detect() we wouldn't be able to get the modes from the EDID in that case. I guess one solution would be to implement the ->force() hook and attempt the edid read there. Looking at the code we should always do a ->detect() or ->force() before ->get_modes(). > = > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/i915/intel_dp.c | 95 ++++++++++++----------------------= ------ > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 29 insertions(+), 67 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel= _dp.c > index ed37407..1eef55c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3764,27 +3764,10 @@ intel_dp_get_edid(struct drm_connector *connector= , struct i2c_adapter *adapter) > return drm_get_edid(connector, adapter); > } > = > -static int > -intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adap= ter *adapter) > -{ > - struct intel_connector *intel_connector =3D to_intel_connector(connecto= r); > - > - /* use cached edid if we have one */ > - if (intel_connector->edid) { > - /* invalid edid */ > - if (IS_ERR(intel_connector->edid)) > - return 0; > - > - return intel_connector_update_modes(connector, > - intel_connector->edid); > - } > - > - return intel_ddc_get_modes(connector, adapter); > -} > - > static enum drm_connector_status > intel_dp_detect(struct drm_connector *connector, bool force) > { > + struct intel_connector *intel_connector =3D to_intel_connector(connecto= r); > struct intel_dp *intel_dp =3D intel_attached_dp(connector); > struct intel_digital_port *intel_dig_port =3D dp_to_dig_port(intel_dp); > struct intel_encoder *intel_encoder =3D &intel_dig_port->base; > @@ -3795,21 +3778,20 @@ intel_dp_detect(struct drm_connector *connector, = bool force) > struct edid *edid =3D NULL; > bool ret; > = > - power_domain =3D intel_display_port_power_domain(intel_encoder); > - intel_display_power_get(dev_priv, power_domain); > - > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > connector->base.id, connector->name); > + kfree(intel_connector->detect_edid); > + intel_connector->detect_edid =3D NULL; > = > if (intel_dp->is_mst) { > /* MST devices are disconnected from a monitor POV */ > if (intel_encoder->type !=3D INTEL_OUTPUT_EDP) > intel_encoder->type =3D INTEL_OUTPUT_DISPLAYPORT; > - status =3D connector_status_disconnected; > - goto out; > + return connector_status_disconnected; > } > = > - intel_dp->has_audio =3D false; > + power_domain =3D intel_display_port_power_domain(intel_encoder); > + intel_display_power_get(dev_priv, power_domain); > = > if (HAS_PCH_SPLIT(dev)) > status =3D ironlake_dp_detect(intel_dp); > @@ -3831,15 +3813,13 @@ intel_dp_detect(struct drm_connector *connector, = bool force) > goto out; > } > = > - if (intel_dp->force_audio !=3D HDMI_AUDIO_AUTO) { > + edid =3D intel_dp_get_edid(connector, &intel_dp->aux.ddc); > + intel_connector->detect_edid =3D edid; > + > + if (intel_dp->force_audio !=3D HDMI_AUDIO_AUTO) > intel_dp->has_audio =3D (intel_dp->force_audio =3D=3D HDMI_AUDIO_ON); > - } else { > - edid =3D intel_dp_get_edid(connector, &intel_dp->aux.ddc); > - if (edid) { > - intel_dp->has_audio =3D drm_detect_monitor_audio(edid); > - kfree(edid); > - } > - } > + else > + intel_dp->has_audio =3D drm_detect_monitor_audio(edid); > = > if (intel_encoder->type !=3D INTEL_OUTPUT_EDP) > intel_encoder->type =3D INTEL_OUTPUT_DISPLAYPORT; > @@ -3852,61 +3832,41 @@ out: > = > static int intel_dp_get_modes(struct drm_connector *connector) > { > - struct intel_dp *intel_dp =3D intel_attached_dp(connector); > - struct intel_digital_port *intel_dig_port =3D dp_to_dig_port(intel_dp); > - struct intel_encoder *intel_encoder =3D &intel_dig_port->base; > struct intel_connector *intel_connector =3D to_intel_connector(connecto= r); > - struct drm_device *dev =3D connector->dev; > - struct drm_i915_private *dev_priv =3D dev->dev_private; > - enum intel_display_power_domain power_domain; > - int ret; > - > - /* We should parse the EDID data and find out if it has an audio sink > - */ > - > - power_domain =3D intel_display_port_power_domain(intel_encoder); > - intel_display_power_get(dev_priv, power_domain); > + struct edid *edid; > = > - ret =3D intel_dp_get_edid_modes(connector, &intel_dp->aux.ddc); > - intel_display_power_put(dev_priv, power_domain); > - if (ret) > - return ret; > + edid =3D intel_connector->detect_edid; > + if (edid) { > + int ret =3D intel_connector_update_modes(connector, edid); > + if (ret) > + return ret; > + } > = > /* if eDP has no EDID, fall back to fixed mode */ > - if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) { > + if (is_edp(intel_attached_dp(connector)) && > + intel_connector->panel.fixed_mode) { > struct drm_display_mode *mode; > - mode =3D drm_mode_duplicate(dev, > + > + mode =3D drm_mode_duplicate(connector->dev, > intel_connector->panel.fixed_mode); > if (mode) { > drm_mode_probed_add(connector, mode); > return 1; > } > } > + > return 0; > } > = > static bool > intel_dp_detect_audio(struct drm_connector *connector) > { > - struct intel_dp *intel_dp =3D intel_attached_dp(connector); > - struct intel_digital_port *intel_dig_port =3D dp_to_dig_port(intel_dp); > - struct intel_encoder *intel_encoder =3D &intel_dig_port->base; > - struct drm_device *dev =3D connector->dev; > - struct drm_i915_private *dev_priv =3D dev->dev_private; > - enum intel_display_power_domain power_domain; > - struct edid *edid; > bool has_audio =3D false; > + struct edid *edid; > = > - power_domain =3D intel_display_port_power_domain(intel_encoder); > - intel_display_power_get(dev_priv, power_domain); > - > - edid =3D intel_dp_get_edid(connector, &intel_dp->aux.ddc); > - if (edid) { > + edid =3D to_intel_connector(connector)->detect_edid; > + if (edid) > has_audio =3D drm_detect_monitor_audio(edid); > - kfree(edid); > - } > - > - intel_display_power_put(dev_priv, power_domain); > = > return has_audio; > } > @@ -4004,6 +3964,7 @@ intel_dp_connector_destroy(struct drm_connector *co= nnector) > { > struct intel_connector *intel_connector =3D to_intel_connector(connecto= r); > = > + kfree(intel_connector->detect_edid); > if (!IS_ERR_OR_NULL(intel_connector->edid)) > kfree(intel_connector->edid); > = > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/inte= l_drv.h > index a031bbf..bb324ac 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -205,6 +205,7 @@ struct intel_connector { > = > /* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */ > struct edid *edid; > + struct edid *detect_edid; > = > /* since POLL and HPD connectors may use the same HPD line keep the nat= ive > state of connector->polled in case hotplug storm detection changes i= t */ > -- = > 1.9.1 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC