Re: [PATCH] drm/i915: Add hot_plug hook for hdmi encoder

["Jindal, Sonika" <sonika.jindal@intel.com>]

On 9/28/2015 7:04 PM, Daniel Vetter wrote:
> On Mon, Sep 28, 2015 at 02:26:04PM +0530, Sonika Jindal wrote:
>> This patch adds a separate probe function for HDMI
>> EDID read over DDC channel. This function has been
>> registered as a .hot_plug handler for HDMI encoder.
>>
>> The current implementation of hdmi_detect()
>> function re-sets the cached HDMI edid (in connector->detect_edid) in
>> every detect call.This function gets called many times, sometimes
>> directly from userspace probes, forcing drivers to read EDID every
>> detect function call.This causes several problems like:
>>
>> 1. Race conditions in multiple hot_plug / unplug cases, between
>>     interrupts bottom halves and userspace detections.
>> 2. Many Un-necessary EDID reads for single hotplug/unplug
>> 3. HDMI complaince failures which expects only one EDID read per hotplug
>>
>> This function will be serving the purpose of really reading the EDID
>> by really probing the DDC channel, and updating the cached EDID.
>>
>> The plan is to:
>> 1. i915 IRQ handler bottom half function already calls
>>     intel_encoder->hotplug() function. Adding This probe function which
>>     will read the EDID only in case of a hotplug / unplug.
>> 2. During init_connector this probe will be called to read the edid
>> 3. Reuse the cached EDID in hdmi_detect() function.
>>
>> The "< gen7" check is there because this was tested only for >=gen7
>> platforms. For older platforms the hotplug/reading edid path remains same.
>>
>> v2: Calling set_edid instead of hdmi_probe during init.
>> Also, for platforms having DDI, intel_encoder for DP and HDMI is same
>> (taken from intel_dig_port), so for DP also, hot_plug function gets called
>> which is not intended here. So, check for HDMI in intel_hdmi_probe
>> Rely on HPD for updating edid only for platforms gen > 8 and also for VLV.
>>
>> v3: Dropping the gen < 8 || !VLV  check. Now all platforms should rely on
>> hotplug or init for updating the edid.(Daniel)
>> Also, calling hdmi_probe in init instead of set_edid
>>
>> v4: Renaming intel_hdmi_probe to intel_hdmi_hot_plug and changing the patch
>> subject. Also calling this hotplug handler from intel_hpd_init to take care
>> of init resume scenarios.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_hdmi.c    |   54 +++++++++++++++++++++++++++-------
>>   drivers/gpu/drm/i915/intel_hotplug.c |   11 +++++++
>>   2 files changed, 54 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index bb33c66..9c1a308 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -1369,18 +1369,16 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>>   	return connected;
>>   }
>>
>> -static enum drm_connector_status
>> -intel_hdmi_detect(struct drm_connector *connector, bool force)
>> +void intel_hdmi_hot_plug(struct intel_encoder *intel_encoder)
>>   {
>> -	enum drm_connector_status status;
>> -	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>> -	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>> +	struct intel_hdmi *intel_hdmi =
>> +			enc_to_intel_hdmi(&intel_encoder->base);
>> +	struct intel_connector *intel_connector =
>> +				intel_hdmi->attached_connector;
>> +	struct drm_i915_private *dev_priv = to_i915(intel_encoder->base.dev);
>>   	bool live_status = false;
>>   	unsigned int retry = 3;
>>
>> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>> -		      connector->base.id, connector->name);
>> -
>>   	while (!live_status && --retry) {
>>   		live_status = intel_digital_port_connected(dev_priv,
>>   				hdmi_to_dig_port(intel_hdmi));
>> @@ -1390,15 +1388,48 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>>   	if (!live_status)
>>   		DRM_DEBUG_KMS("Live status not up!");
>>
>> -	intel_hdmi_unset_edid(connector);
>> +	/*
>> +	 * We are here, means there is a hotplug or a force
>> +	 * detection. Clear the cached EDID and probe the
>> +	 * DDC bus to check the current status of HDMI.
>> +	 */
>> +	intel_hdmi_unset_edid(&intel_connector->base);
>> +	if (intel_hdmi_set_edid(&intel_connector->base, live_status))
>> +		DRM_DEBUG_DRIVER("DDC probe: got EDID\n");
>> +	else
>> +		DRM_DEBUG_DRIVER("DDC probe: no EDID\n");
>> +}
>>
>> -	if (intel_hdmi_set_edid(connector, live_status)) {
>> +static enum drm_connector_status
>> +intel_hdmi_detect(struct drm_connector *connector, bool force)
>> +{
>> +	enum drm_connector_status status;
>> +	struct intel_connector *intel_connector =
>> +				to_intel_connector(connector);
>> +
>> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>> +		      connector->base.id, connector->name);
>> +
>> +	/*
>> +	 * There are many userspace calls which probe EDID from
>> +	 * detect path. In case of multiple hotplug/unplug, these
>> +	 * can cause race conditions while probing EDID. Also its
>> +	 * waste of CPU cycles to read the EDID again and again
>> +	 * unless there is a real hotplug.
>> +	 * So, rely on hotplugs and init to read edid.
>> +	 * Check connector status based on availability of cached EDID.
>> +	 */
>> +
>> +	if (intel_connector->detect_edid) {
>>   		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
>> +		DRM_DEBUG_DRIVER("hdmi status = connected\n");
>> +	} else {
>>   		status = connector_status_disconnected;
>> +		DRM_DEBUG_DRIVER("hdmi status = disconnected\n");
>> +	}
>>
>>   	return status;
>>   }
>> @@ -2131,6 +2162,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>   	intel_connector->unregister = intel_connector_unregister;
>>
>>   	intel_hdmi_add_properties(intel_hdmi, connector);
>> +	intel_encoder->hot_plug = intel_hdmi_hot_plug;
>>
>>   	intel_connector_attach_encoder(intel_connector, intel_encoder);
>>   	drm_connector_register(connector);
>> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
>> index 53c0173..36e16f6 100644
>> --- a/drivers/gpu/drm/i915/intel_hotplug.c
>> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
>> @@ -459,6 +459,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>>   	struct drm_device *dev = dev_priv->dev;
>>   	struct drm_mode_config *mode_config = &dev->mode_config;
>>   	struct drm_connector *connector;
>> +	struct intel_encoder *encoder;
>>   	int i;
>>
>>   	for_each_hpd_pin(i) {
>> @@ -482,6 +483,16 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>>   	if (dev_priv->display.hpd_irq_setup)
>>   		dev_priv->display.hpd_irq_setup(dev);
>>   	spin_unlock_irq(&dev_priv->irq_lock);
>> +
>> +	/*
>> +	 * Connected boot / resume scenarios can't generate new hot plug.
>> +	 * So, probe it manually.
>> +	 */
>> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
>> +			    base.head) {
>> +		if (encoder->hot_plug)
>> +			encoder->hot_plug(encoder);
>> +	}
>
> Since this doesn't just change hdmi but every encoder I think we should
> pull it out into a separate prep patch. Especially interactions with dp
> mst handling look tricky, have you tested that nothing blows up with that?
> The split-out patch should definitely explain in detail how this interacts
> with dp hpd handling.
> -Daniel
>
Right now we don't set hot_plug for DP/edp encoder.
It was used only for sdvo and with this patch we add for hdmi.
Also, in sdvo_init right after assigning the hot_plug function pointer, 
we call its hot_plug function for initialization.
Maybe with this patch, we can remove that part also from sdvo_init.

I can split this patch but without this change, it will result in 
regression with just one patch. So I thought it would be better to keep 
them together in one patch.

Regards,
Sonika

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx