Re: [PATCH 3/3] drm/i915/hdmi: Cache EDID for a detection cycle

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 2 Sep 2014 13:45:37 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Tue, Sep 02, 2014 at 09:24:48AM +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.
> > 
> > v2: Also hookup the force() callback for audio detection when the
> > user forces the connection status.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 145
> > +++++++++++++++++++++----------------- 1 file changed, 80
> > insertions(+), 65 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c index
> > 9169786dbbc3..7428521d2e25 100644 ---
> > a/drivers/gpu/drm/i915/intel_hdmi.c +++
> > b/drivers/gpu/drm/i915/intel_hdmi.c @@ -962,104 +962,117 @@ bool
> > intel_hdmi_compute_config(struct intel_encoder *encoder, return
> > true; }
> >  
> > -static enum drm_connector_status
> > -intel_hdmi_detect(struct drm_connector *connector, bool force)
> > +static void
> > +intel_hdmi_unset_edid(struct drm_connector *connector)
> >  {
> > -	struct drm_device *dev = connector->dev;
> >  	struct intel_hdmi *intel_hdmi =
> > intel_attached_hdmi(connector);
> > -	struct intel_digital_port *intel_dig_port =
> > -		hdmi_to_dig_port(intel_hdmi);
> > -	struct intel_encoder *intel_encoder =
> > &intel_dig_port->base;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct edid *edid;
> > -	enum intel_display_power_domain power_domain;
> > -	enum drm_connector_status status =
> > connector_status_disconnected; 
> > -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > -		      connector->base.id, connector->name);
> > +	intel_hdmi->has_hdmi_sink = false;
> > +	intel_hdmi->has_audio = false;
> > +	intel_hdmi->rgb_quant_range_selectable = false;
> > +
> > +	kfree(to_intel_connector(connector)->detect_edid);
> > +	to_intel_connector(connector)->detect_edid = NULL;
> > +}
> > +
> > +static bool
> > +intel_hdmi_set_edid(struct drm_connector *connector)
> > +{
> > +	struct drm_i915_private *dev_priv =
> > to_i915(connector->dev);
> > +	struct intel_hdmi *intel_hdmi =
> > intel_attached_hdmi(connector);
> > +	struct intel_encoder *intel_encoder =
> > +		&hdmi_to_dig_port(intel_hdmi)->base;
> > +	enum intel_display_power_domain power_domain;
> > +	struct edid *edid;
> > +	bool connected = false;
> >  
> >  	power_domain =
> > intel_display_port_power_domain(intel_encoder);
> > intel_display_power_get(dev_priv, power_domain); 
> > -	intel_hdmi->has_hdmi_sink = false;
> > -	intel_hdmi->has_audio = false;
> > -	intel_hdmi->rgb_quant_range_selectable = false;
> >  	edid = drm_get_edid(connector,
> >  			    intel_gmbus_get_adapter(dev_priv,
> >  						    intel_hdmi->ddc_bus));
> >  
> > -	if (edid) {
> > -		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> > -			status = connector_status_connected;
> > -			if (intel_hdmi->force_audio !=
> > HDMI_AUDIO_OFF_DVI)
> > -				intel_hdmi->has_hdmi_sink =
> > -
> > drm_detect_hdmi_monitor(edid);
> > -			intel_hdmi->has_audio =
> > drm_detect_monitor_audio(edid);
> > -			intel_hdmi->rgb_quant_range_selectable =
> > -
> > drm_rgb_quant_range_selectable(edid);
> > -		}
> > -		kfree(edid);
> > -	}
> > +	intel_display_power_put(dev_priv, power_domain);
> > +
> > +	to_intel_connector(connector)->detect_edid = edid;
> > +	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> > +		intel_hdmi->rgb_quant_range_selectable =
> > +			drm_rgb_quant_range_selectable(edid);
> >  
> > -	if (status == connector_status_connected) {
> > +		intel_hdmi->has_audio =
> > drm_detect_monitor_audio(edid); if (intel_hdmi->force_audio !=
> > HDMI_AUDIO_AUTO) intel_hdmi->has_audio =
> > -				(intel_hdmi->force_audio ==
> > HDMI_AUDIO_ON);
> > -		intel_encoder->type = INTEL_OUTPUT_HDMI;
> > +				intel_hdmi->force_audio ==
> > HDMI_AUDIO_ON; +
> > +		if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
> > +			intel_hdmi->has_hdmi_sink =
> > +				drm_detect_hdmi_monitor(edid);
> 
> I wonder if we should be updating has_hdmi_sink also in
> intel_hdmi_set_property() when force_audio != HDMI_AUDIO_OFF_DVI.
> But that's a separate issue so material for another patch.
> 
> > +
> > +		connected = true;
> >  	}
> >  
> > -	intel_display_power_put(dev_priv, power_domain);
> > +	return connected;
> > +}
> > +
> > +static enum drm_connector_status
> > +intel_hdmi_detect(struct drm_connector *connector, bool force)
> > +{
> > +	enum drm_connector_status status;
> > +
> > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > +		      connector->base.id, connector->name);
> > +
> > +	intel_hdmi_unset_edid(connector);
> > +
> > +	if (intel_hdmi_set_edid(connector)) {
> > +		struct intel_hdmi *intel_hdmi =
> > intel_attached_hdmi(connector); +
> > +		hdmi_to_dig_port(intel_hdmi)->base.type =
> > INTEL_OUTPUT_HDMI;
> > +		status = connector_status_connected;
> > +	} else
> > +		status = connector_status_disconnected;
> >  
> >  	return status;
> >  }
> >  
> > -static int intel_hdmi_get_modes(struct drm_connector *connector)
> > +static void
> > +intel_hdmi_force(struct drm_connector *connector)
> >  {
> > -	struct intel_encoder *intel_encoder =
> > intel_attached_encoder(connector);
> > -	struct intel_hdmi *intel_hdmi =
> > enc_to_intel_hdmi(&intel_encoder->base);
> > -	struct drm_i915_private *dev_priv =
> > connector->dev->dev_private;
> > -	enum intel_display_power_domain power_domain;
> > -	int ret;
> > +	struct intel_hdmi *intel_hdmi =
> > intel_attached_hdmi(connector); 
> > -	/* We should parse the EDID data and find out if it's an
> > HDMI sink so
> > -	 * we can send audio to it.
> > -	 */
> > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > +		      connector->base.id, connector->name);
> >  
> > -	power_domain =
> > intel_display_port_power_domain(intel_encoder);
> > -	intel_display_power_get(dev_priv, power_domain);
> > +	intel_hdmi_unset_edid(connector);
> >  
> > -	ret = intel_ddc_get_modes(connector,
> > -
> > intel_gmbus_get_adapter(dev_priv,
> > -
> > intel_hdmi->ddc_bus));
> > +	if (connector->status == connector_status_connected)
> 
> !=
> 
> Apart from that I didn't spot anything suspicious. So with this one
> thing fixed you can slap on
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> for the series.
> 
> > +		return;
> >  
> > -	intel_display_power_put(dev_priv, power_domain);
> > +	intel_hdmi_set_edid(connector);
> > +	hdmi_to_dig_port(intel_hdmi)->base.type =
> > INTEL_OUTPUT_HDMI; +}
> >  
> > -	return ret;
> > +static int intel_hdmi_get_modes(struct drm_connector *connector)
> > +{
> > +	struct edid *edid;
> > +
> > +	edid = to_intel_connector(connector)->detect_edid;
> > +	if (edid == NULL)
> > +		return 0;
> > +
> > +	return intel_connector_update_modes(connector, edid);
> >  }
> >  
> >  static bool
> >  intel_hdmi_detect_audio(struct drm_connector *connector)
> >  {
> > -	struct intel_encoder *intel_encoder =
> > intel_attached_encoder(connector);
> > -	struct intel_hdmi *intel_hdmi =
> > enc_to_intel_hdmi(&intel_encoder->base);
> > -	struct drm_i915_private *dev_priv =
> > connector->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 = drm_get_edid(connector,
> > -			    intel_gmbus_get_adapter(dev_priv,
> > -
> > intel_hdmi->ddc_bus));
> > -	if (edid) {
> > -		if (edid->input & DRM_EDID_INPUT_DIGITAL)
> > -			has_audio = drm_detect_monitor_audio(edid);
> > -		kfree(edid);
> > -	}
> > -
> > -	intel_display_power_put(dev_priv, power_domain);
> > +	edid = to_intel_connector(connector)->detect_edid;
> > +	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL)
> > +		has_audio = drm_detect_monitor_audio(edid);
> >  
> >  	return has_audio;
> >  }
> > @@ -1479,6 +1492,7 @@ static void chv_hdmi_pre_enable(struct
> > intel_encoder *encoder) 
> >  static void intel_hdmi_destroy(struct drm_connector *connector)
> >  {
> > +	intel_hdmi_unset_edid(connector);
> >  	drm_connector_cleanup(connector);
> >  	kfree(connector);
> >  }
> > @@ -1486,6 +1500,7 @@ static void intel_hdmi_destroy(struct
> > drm_connector *connector) static const struct drm_connector_funcs
> > intel_hdmi_connector_funcs = { .dpms = intel_connector_dpms,
> >  	.detect = intel_hdmi_detect,
> > +	.force = intel_hdmi_force,
> >  	.fill_modes = drm_helper_probe_single_connector_modes,
> >  	.set_property = intel_hdmi_set_property,
> >  	.destroy = intel_hdmi_destroy,
> > -- 
> > 2.1.0
> 


Tried to find a JIRA for this but only came up with VIZ-1789.  Looks
like it might be close enough for Wayland?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx