diff mbox

drm/i915: Add hot_plug hook for hdmi encoder

Message ID 1443430564-12547-1-git-send-email-sonika.jindal@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

sonika.jindal@intel.com Sept. 28, 2015, 8:56 a.m. UTC
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(-)

Comments

Daniel Vetter Sept. 28, 2015, 1:34 p.m. UTC | #1
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
sonika.jindal@intel.com Sept. 29, 2015, 4:13 a.m. UTC | #2
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
Daniel Vetter Sept. 29, 2015, 9:04 a.m. UTC | #3
On Tue, Sep 29, 2015 at 09:43:10AM +0530, Jindal, Sonika wrote:
> 
> 
> 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.

Hm right, I forgot that we have too many hotplug callbacks in our driver
;-)

> 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.

If you split it out and make it patch 1 and then the hdmi changes patch 2
nothing breaks in between.
-Daniel
diff mbox

Patch

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);
+	}
 }
 
 void intel_hpd_init_work(struct drm_i915_private *dev_priv)