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 <chris@xxxxxxxxxxxxxxxxxx>
> ---
> 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_adapter *adapter)
> -{
> - struct intel_connector *intel_connector = to_intel_connector(connector);
> -
> - /* 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 = to_intel_connector(connector);
> struct intel_dp *intel_dp = intel_attached_dp(connector);
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> struct intel_encoder *intel_encoder = &intel_dig_port->base;
> @@ -3795,21 +3778,20 @@ intel_dp_detect(struct drm_connector *connector, bool force)
> struct edid *edid = NULL;
> bool ret;
>
> - power_domain = 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 = NULL;
>
> if (intel_dp->is_mst) {
> /* MST devices are disconnected from a monitor POV */
> if (intel_encoder->type != INTEL_OUTPUT_EDP)
> intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> - status = connector_status_disconnected;
> - goto out;
> + return connector_status_disconnected;
> }
>
> - intel_dp->has_audio = false;
> + power_domain = intel_display_port_power_domain(intel_encoder);
> + intel_display_power_get(dev_priv, power_domain);
>
> if (HAS_PCH_SPLIT(dev))
> status = 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 != HDMI_AUDIO_AUTO) {
> + edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
> + intel_connector->detect_edid = edid;
> +
> + if (intel_dp->force_audio != HDMI_AUDIO_AUTO)
> intel_dp->has_audio = (intel_dp->force_audio == HDMI_AUDIO_ON);
> - } else {
> - edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
> - if (edid) {
> - intel_dp->has_audio = drm_detect_monitor_audio(edid);
> - kfree(edid);
> - }
> - }
> + else
> + intel_dp->has_audio = drm_detect_monitor_audio(edid);
>
> if (intel_encoder->type != INTEL_OUTPUT_EDP)
> intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> @@ -3852,61 +3832,41 @@ out:
>
> static int intel_dp_get_modes(struct drm_connector *connector)
> {
> - struct intel_dp *intel_dp = intel_attached_dp(connector);
> - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> - struct intel_encoder *intel_encoder = &intel_dig_port->base;
> struct intel_connector *intel_connector = to_intel_connector(connector);
> - struct drm_device *dev = connector->dev;
> - struct drm_i915_private *dev_priv = 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 = intel_display_port_power_domain(intel_encoder);
> - intel_display_power_get(dev_priv, power_domain);
> + struct edid *edid;
>
> - ret = intel_dp_get_edid_modes(connector, &intel_dp->aux.ddc);
> - intel_display_power_put(dev_priv, power_domain);
> - if (ret)
> - return ret;
> + edid = intel_connector->detect_edid;
> + if (edid) {
> + int ret = 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 = drm_mode_duplicate(dev,
> +
> + mode = 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 = intel_attached_dp(connector);
> - struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> - struct intel_encoder *intel_encoder = &intel_dig_port->base;
> - struct drm_device *dev = connector->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - enum intel_display_power_domain power_domain;
> - struct edid *edid;
> bool has_audio = false;
> + struct edid *edid;
>
> - power_domain = intel_display_port_power_domain(intel_encoder);
> - intel_display_power_get(dev_priv, power_domain);
> -
> - edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
> - if (edid) {
> + edid = to_intel_connector(connector)->detect_edid;
> + if (edid)
> has_audio = 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 *connector)
> {
> struct intel_connector *intel_connector = to_intel_connector(connector);
>
> + 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/intel_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 native
> state of connector->polled in case hotplug storm detection changes it */
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
