[PATCH 1/3] drm/i915: Split encoder->hot_plug() into pre and post variants

[PATCH 1/3] drm/i915: Split encoder->hot_plug() into pre and post variants

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

During hpd processing we may want to do things both before and
after the display detection. To that end split the encoder->hot_plug()
hooks.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h     | 3 ++-
 drivers/gpu/drm/i915/intel_hotplug.c | 6 ++++--
 drivers/gpu/drm/i915/intel_sdvo.c    | 2 +-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 30f791f89d64..4a7e603ccc38 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -211,7 +211,8 @@ struct intel_encoder {
 	enum intel_output_type type;
 	enum port port;
 	unsigned int cloneable;
-	void (*hot_plug)(struct intel_encoder *);
+	void (*pre_hotplug)(struct intel_encoder *encoder);
+	void (*post_hotplug)(struct intel_encoder *encoder);
 	enum intel_output_type (*compute_output_type)(struct intel_encoder *,
 						      struct intel_crtc_state *,
 						      struct drm_connector_state *);
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 875d5d218d5c..91abca3ae11a 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -370,10 +370,12 @@ static void i915_hotplug_work_func(struct work_struct *work)
 		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
 			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
 				      connector->name, intel_encoder->hpd_pin);
-			if (intel_encoder->hot_plug)
-				intel_encoder->hot_plug(intel_encoder);
+			if (intel_encoder->pre_hotplug)
+				intel_encoder->pre_hotplug(intel_encoder);
 			if (intel_hpd_irq_event(dev, connector))
 				changed = true;
+			if (intel_encoder->post_hotplug)
+				intel_encoder->post_hotplug(intel_encoder);
 		}
 	}
 	drm_connector_list_iter_end(&conn_iter);
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 2b8764897d68..6de90a1fc553 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2496,7 +2496,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 		/* Some SDVO devices have one-shot hotplug interrupts.
 		 * Ensure that they get re-enabled when an interrupt happens.
 		 */
-		intel_encoder->hot_plug = intel_sdvo_enable_hotplug;
+		intel_encoder->pre_hotplug = intel_sdvo_enable_hotplug;
 		intel_sdvo_enable_hotplug(intel_encoder);
 	} else {
 		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
-- 
2.13.6

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

[PATCH 2/3] drm/i915: Reinitialize sink scrambling/TMDS clock ratio on HPD

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The LG 4k TV I have doesn't deassert HPD when I turn the TV off, but
when I turn it back on it will pulse the HPD line. By that time it has
forgotten everything we told it about scrambling and the clock ratio.
Hence if we want to get a picture out if it again we have to tell it
whether we're currently sending scrambled data or not. Implement
that via the encoder->post_hotplug() hook.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 75 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index f51645a08dca..12da7024f01a 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2720,6 +2720,79 @@ intel_ddi_init_dp_connector(struct intel_digital_port *intel_dig_port)
 	return connector;
 }
 
+static int intel_ddi_hdmi_reset_scrambling(struct intel_encoder *encoder,
+					   struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_hdmi *hdmi = enc_to_intel_hdmi(&encoder->base);
+	struct intel_connector *connector = hdmi->attached_connector;
+	struct drm_connector_state *conn_state;
+	struct intel_crtc_state *crtc_state;
+	struct intel_crtc *crtc;
+	int ret;
+
+	if (!connector || connector->base.status != connector_status_connected)
+		return 0;
+
+	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, ctx);
+	if (ret)
+		return ret;
+
+	conn_state = connector->base.state;
+
+	crtc = to_intel_crtc(conn_state->crtc);
+	if (!crtc)
+		return 0;
+
+	ret = drm_modeset_lock(&crtc->base.mutex, ctx);
+	if (ret)
+		return ret;
+
+	crtc_state = to_intel_crtc_state(crtc->base.state);
+
+	WARN_ON(!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI));
+
+	if (!crtc_state->base.active)
+		return 0;
+
+	if (!crtc_state->hdmi_high_tmds_clock_ratio &&
+	    !crtc_state->hdmi_scrambling)
+		return 0;
+
+	if (conn_state->commit &&
+	    !try_wait_for_completion(&conn_state->commit->hw_done))
+		return 0;
+
+	intel_hdmi_handle_sink_scrambling(encoder, &connector->base,
+					  crtc_state->hdmi_high_tmds_clock_ratio,
+					  crtc_state->hdmi_scrambling);
+
+	return 0;
+}
+
+static void intel_ddi_post_hotplug(struct intel_encoder *encoder)
+{
+	struct drm_modeset_acquire_ctx ctx;
+	int ret;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	for (;;) {
+		ret = intel_ddi_hdmi_reset_scrambling(encoder, &ctx);
+
+		if (ret == -EDEADLK) {
+			drm_modeset_backoff(&ctx);
+			continue;
+		}
+
+		break;
+	}
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
+}
+
 static struct intel_connector *
 intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
 {
@@ -2830,6 +2903,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	drm_encoder_init(&dev_priv->drm, encoder, &intel_ddi_funcs,
 			 DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
 
+	if (init_hdmi)
+		intel_encoder->post_hotplug = intel_ddi_post_hotplug;
 	intel_encoder->compute_output_type = intel_ddi_compute_output_type;
 	intel_encoder->compute_config = intel_ddi_compute_config;
 	intel_encoder->enable = intel_enable_ddi;
-- 
2.13.6

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

Re: [PATCH 2/3] drm/i915: Reinitialize sink scrambling/TMDS clock ratio on HPD

[Sharma, Shashank]

On 12/22/2017 11:58 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The LG 4k TV I have doesn't deassert HPD when I turn the TV off, but
> when I turn it back on it will pulse the HPD line. By that time it has
> forgotten everything we told it about scrambling and the clock ratio.
> Hence if we want to get a picture out if it again we have to tell it
> whether we're currently sending scrambled data or not. Implement
> that via the encoder->post_hotplug() hook.
I am not sure if I understood the problem statement correctly. Even if 
the TV triggers HPD line while turning it back, I would expect:
- EDID read for TV's detection, which will refresh SCDC and scrambling 
capabilities
- A new modeset will be triggered, which will program the scrambling and 
high tmds clock ratio again
- Once HDMI controller is programmed, it will generate scrambled signals 
till next modeset / disable.

So why do we need to do this ? I might be missing something, but lets 
discus about it.

- Shashank
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_ddi.c | 75 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 75 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index f51645a08dca..12da7024f01a 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2720,6 +2720,79 @@ intel_ddi_init_dp_connector(struct intel_digital_port *intel_dig_port)
>   	return connector;
>   }
>   
> +static int intel_ddi_hdmi_reset_scrambling(struct intel_encoder *encoder,
> +					   struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_hdmi *hdmi = enc_to_intel_hdmi(&encoder->base);
> +	struct intel_connector *connector = hdmi->attached_connector;
> +	struct drm_connector_state *conn_state;
> +	struct intel_crtc_state *crtc_state;
> +	struct intel_crtc *crtc;
> +	int ret;
> +
> +	if (!connector || connector->base.status != connector_status_connected)
> +		return 0;
> +
> +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, ctx);
> +	if (ret)
> +		return ret;
> +
> +	conn_state = connector->base.state;
> +
> +	crtc = to_intel_crtc(conn_state->crtc);
> +	if (!crtc)
> +		return 0;
> +
> +	ret = drm_modeset_lock(&crtc->base.mutex, ctx);
> +	if (ret)
> +		return ret;
> +
> +	crtc_state = to_intel_crtc_state(crtc->base.state);
> +
> +	WARN_ON(!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI));
> +
> +	if (!crtc_state->base.active)
> +		return 0;
> +
> +	if (!crtc_state->hdmi_high_tmds_clock_ratio &&
> +	    !crtc_state->hdmi_scrambling)
> +		return 0;
> +
> +	if (conn_state->commit &&
> +	    !try_wait_for_completion(&conn_state->commit->hw_done))
> +		return 0;
> +
> +	intel_hdmi_handle_sink_scrambling(encoder, &connector->base,
> +					  crtc_state->hdmi_high_tmds_clock_ratio,
> +					  crtc_state->hdmi_scrambling);
> +
> +	return 0;
> +}
> +
> +static void intel_ddi_post_hotplug(struct intel_encoder *encoder)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	int ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	for (;;) {
> +		ret = intel_ddi_hdmi_reset_scrambling(encoder, &ctx);
> +
> +		if (ret == -EDEADLK) {
> +			drm_modeset_backoff(&ctx);
> +			continue;
> +		}
> +
> +		break;
> +	}
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
> +}
> +
>   static struct intel_connector *
>   intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
>   {
> @@ -2830,6 +2903,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>   	drm_encoder_init(&dev_priv->drm, encoder, &intel_ddi_funcs,
>   			 DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
>   
> +	if (init_hdmi)
> +		intel_encoder->post_hotplug = intel_ddi_post_hotplug;
>   	intel_encoder->compute_output_type = intel_ddi_compute_output_type;
>   	intel_encoder->compute_config = intel_ddi_compute_config;
>   	intel_encoder->enable = intel_enable_ddi;

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

Re: [PATCH 2/3] drm/i915: Reinitialize sink scrambling/TMDS clock ratio on HPD

[Ville Syrjälä]

On Thu, Dec 28, 2017 at 08:32:05PM +0530, Sharma, Shashank wrote:
> 
> 
> On 12/22/2017 11:58 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > The LG 4k TV I have doesn't deassert HPD when I turn the TV off, but
> > when I turn it back on it will pulse the HPD line. By that time it has
> > forgotten everything we told it about scrambling and the clock ratio.
> > Hence if we want to get a picture out if it again we have to tell it
> > whether we're currently sending scrambled data or not. Implement
> > that via the encoder->post_hotplug() hook.
> I am not sure if I understood the problem statement correctly. Even if 
> the TV triggers HPD line while turning it back, I would expect:
> - EDID read for TV's detection, which will refresh SCDC and scrambling 
> capabilities
> - A new modeset will be triggered, which will program the scrambling and 
> high tmds clock ratio again
> - Once HDMI controller is programmed, it will generate scrambled signals 
> till next modeset / disable.
> 
> So why do we need to do this ? I might be missing something, but lets 
> discus about it.

The EDID is readable even when the HPD gets deasserted for a short
perios, hence we never consider the sink as being disconnected. Hence
there will be no modeset triggered by userspace.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Reinitialize sink scrambling/TMDS clock ratio on HPD

[Sharma, Shashank]

Regards

Shashank


On 1/9/2018 11:31 PM, Ville Syrjälä wrote:
> On Thu, Dec 28, 2017 at 08:32:05PM +0530, Sharma, Shashank wrote:
>>
>> On 12/22/2017 11:58 PM, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> The LG 4k TV I have doesn't deassert HPD when I turn the TV off, but
>>> when I turn it back on it will pulse the HPD line. By that time it has
>>> forgotten everything we told it about scrambling and the clock ratio.
>>> Hence if we want to get a picture out if it again we have to tell it
>>> whether we're currently sending scrambled data or not. Implement
>>> that via the encoder->post_hotplug() hook.
>> I am not sure if I understood the problem statement correctly. Even if
>> the TV triggers HPD line while turning it back, I would expect:
>> - EDID read for TV's detection, which will refresh SCDC and scrambling
>> capabilities
>> - A new modeset will be triggered, which will program the scrambling and
>> high tmds clock ratio again
>> - Once HDMI controller is programmed, it will generate scrambled signals
>> till next modeset / disable.
>>
>> So why do we need to do this ? I might be missing something, but lets
>> discus about it.
> The EDID is readable even when the HPD gets deasserted for a short
> perios, hence we never consider the sink as being disconnected. Hence
> there will be no modeset triggered by userspace.
This is a bigger problem then, in that case, the pipe/port would be 
enabled, and IMHO I don't think fixing just the scrambling status is 
right thing to do.
Also, is this the right behavior from the monitor ? I am also worried if 
we are trying to fix the monitor's fault in our driver.

- Shashank

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

Re: [PATCH 2/3] drm/i915: Reinitialize sink scrambling/TMDS clock ratio on HPD

[Ville Syrjälä]

On Wed, Jan 10, 2018 at 10:07:43AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 1/9/2018 11:31 PM, Ville Syrjälä wrote:
> > On Thu, Dec 28, 2017 at 08:32:05PM +0530, Sharma, Shashank wrote:
> >>
> >> On 12/22/2017 11:58 PM, Ville Syrjala wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> The LG 4k TV I have doesn't deassert HPD when I turn the TV off, but
> >>> when I turn it back on it will pulse the HPD line. By that time it has
> >>> forgotten everything we told it about scrambling and the clock ratio.
> >>> Hence if we want to get a picture out if it again we have to tell it
> >>> whether we're currently sending scrambled data or not. Implement
> >>> that via the encoder->post_hotplug() hook.
> >> I am not sure if I understood the problem statement correctly. Even if
> >> the TV triggers HPD line while turning it back, I would expect:
> >> - EDID read for TV's detection, which will refresh SCDC and scrambling
> >> capabilities
> >> - A new modeset will be triggered, which will program the scrambling and
> >> high tmds clock ratio again
> >> - Once HDMI controller is programmed, it will generate scrambled signals
> >> till next modeset / disable.
> >>
> >> So why do we need to do this ? I might be missing something, but lets
> >> discus about it.
> > The EDID is readable even when the HPD gets deasserted for a short
> > perios, hence we never consider the sink as being disconnected. Hence
> > there will be no modeset triggered by userspace.
> This is a bigger problem then, in that case, the pipe/port would be 
> enabled, and IMHO I don't think fixing just the scrambling status is 
> right thing to do.

Hmm. The spec does say "the Source shall not begin transmission of a
scrambled video signal before writing a 1 to the Scrambling_Enable bit".
So I guess you're right. We can't follow that rule 100% though because
we can't detect that the the sink has been turned off.

If we checked the live hpd status during hpd processing we should be
able to detect that the sink was logically disconnected for a short
period, but as we know the live hpd status is not exactly reliable
for HDMI. Also that would still be racy on account of hpd processing
delays.

When talking about the TMDS clock ratio the spec says that we have to
suspend TMDS clock/data transmission when we change the TMDS clock ratio
setting in the sink.

So I guess what we could do is force a full modeset if and when the sink
has become confused about these settings. Or if we want to optimize
things a bit I suppose we could just turn off DDI_BUF_CTL around the
operation. But probably no point in optimizing that part since it's a
rare event.

> Also, is this the right behavior from the monitor ? I am also worried if 
> we are trying to fix the monitor's fault in our driver.

I don't think that's specified anywhere. Also doesn't matter because we
have to fix sink specific issues in the driver all the time. Otherwise
a lot of sinks would just fail to work.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Reinitialize sink scrambling/TMDS clock ratio on HPD

[Sharma, Shashank]

Regards

Shashank


On 1/10/2018 9:53 PM, Ville Syrjälä wrote:
> On Wed, Jan 10, 2018 at 10:07:43AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 1/9/2018 11:31 PM, Ville Syrjälä wrote:
>>> On Thu, Dec 28, 2017 at 08:32:05PM +0530, Sharma, Shashank wrote:
>>>> On 12/22/2017 11:58 PM, Ville Syrjala wrote:
>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>
>>>>> The LG 4k TV I have doesn't deassert HPD when I turn the TV off, but
>>>>> when I turn it back on it will pulse the HPD line. By that time it has
>>>>> forgotten everything we told it about scrambling and the clock ratio.
>>>>> Hence if we want to get a picture out if it again we have to tell it
>>>>> whether we're currently sending scrambled data or not. Implement
>>>>> that via the encoder->post_hotplug() hook.
>>>> I am not sure if I understood the problem statement correctly. Even if
>>>> the TV triggers HPD line while turning it back, I would expect:
>>>> - EDID read for TV's detection, which will refresh SCDC and scrambling
>>>> capabilities
>>>> - A new modeset will be triggered, which will program the scrambling and
>>>> high tmds clock ratio again
>>>> - Once HDMI controller is programmed, it will generate scrambled signals
>>>> till next modeset / disable.
>>>>
>>>> So why do we need to do this ? I might be missing something, but lets
>>>> discus about it.
>>> The EDID is readable even when the HPD gets deasserted for a short
>>> perios, hence we never consider the sink as being disconnected. Hence
>>> there will be no modeset triggered by userspace.
>> This is a bigger problem then, in that case, the pipe/port would be
>> enabled, and IMHO I don't think fixing just the scrambling status is
>> right thing to do.
> Hmm. The spec does say "the Source shall not begin transmission of a
> scrambled video signal before writing a 1 to the Scrambling_Enable bit".
> So I guess you're right. We can't follow that rule 100% though because
> we can't detect that the the sink has been turned off.
>
> If we checked the live hpd status during hpd processing we should be
> able to detect that the sink was logically disconnected for a short
> period, but as we know the live hpd status is not exactly reliable
> for HDMI. Also that would still be racy on account of hpd processing
> delays.
Agree. This inaccuracy of Live status HW has been a nightmare for us 
since a long time,
it really cripples proper hotplug handling.
>
> When talking about the TMDS clock ratio the spec says that we have to
> suspend TMDS clock/data transmission when we change the TMDS clock ratio
> setting in the sink.
>
> So I guess what we could do is force a full modeset if and when the sink
> has become confused about these settings. Or if we want to optimize
> things a bit I suppose we could just turn off DDI_BUF_CTL around the
> operation. But probably no point in optimizing that part since it's a
> rare event.
Agree, again. Also it would be difficult to detect exactly when do we 
have a genuine confusion :-)

- Shashank
>
>> Also, is this the right behavior from the monitor ? I am also worried if
>> we are trying to fix the monitor's fault in our driver.
> I don't think that's specified anywhere. Also doesn't matter because we
> have to fix sink specific issues in the driver all the time. Otherwise
> a lot of sinks would just fail to work.
>

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

[PATCH 3/3] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Doing link reatrining from the short pulse handler is problematic since
that might introduce deadlocks with MST sideband processing. Currently
we don't retrain MST links from this code, but we want to change that.
So better to move the entire thing to the hotplug work. We can utilize
the new encoder->post_hotplug() hook for this.

The only thing we leave in the short pulse handler is the link status
check. That one still depends on the link parameters stored under
intel_dp, so no locking around that but races should be mostly harmless
as the actual retraining code will recheck the link state if we
end up there by mistake.

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c |   7 +-
 drivers/gpu/drm/i915/intel_dp.c  | 213 ++++++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_drv.h |   2 +
 3 files changed, 125 insertions(+), 97 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 12da7024f01a..7e9964794efe 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2778,7 +2778,9 @@ static void intel_ddi_post_hotplug(struct intel_encoder *encoder)
 	drm_modeset_acquire_init(&ctx, 0);
 
 	for (;;) {
-		ret = intel_ddi_hdmi_reset_scrambling(encoder, &ctx);
+		ret = intel_dp_retrain_link(encoder, &ctx);
+		if (!ret)
+			ret = intel_ddi_hdmi_reset_scrambling(encoder, &ctx);
 
 		if (ret == -EDEADLK) {
 			drm_modeset_backoff(&ctx);
@@ -2903,8 +2905,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	drm_encoder_init(&dev_priv->drm, encoder, &intel_ddi_funcs,
 			 DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
 
-	if (init_hdmi)
-		intel_encoder->post_hotplug = intel_ddi_post_hotplug;
+	intel_encoder->post_hotplug = intel_ddi_post_hotplug;
 	intel_encoder->compute_output_type = intel_ddi_compute_output_type;
 	intel_encoder->compute_config = intel_ddi_compute_config;
 	intel_encoder->enable = intel_enable_ddi;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 35c5299feab6..72234c5dc15b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4251,74 +4251,137 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 	return -EINVAL;
 }
 
-static void
-intel_dp_retrain_link(struct intel_dp *intel_dp)
+static bool
+intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
 {
-	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
-
-	/* Suppress underruns caused by re-training */
-	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
-	if (crtc->config->has_pch_encoder)
-		intel_set_pch_fifo_underrun_reporting(dev_priv,
-						      intel_crtc_pch_transcoder(crtc), false);
-
-	intel_dp_start_link_train(intel_dp);
-	intel_dp_stop_link_train(intel_dp);
-
-	/* Keep underrun reporting disabled until things are stable */
-	intel_wait_for_vblank(dev_priv, crtc->pipe);
-
-	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true);
-	if (crtc->config->has_pch_encoder)
-		intel_set_pch_fifo_underrun_reporting(dev_priv,
-						      intel_crtc_pch_transcoder(crtc), true);
-}
-
-static void
-intel_dp_check_link_status(struct intel_dp *intel_dp)
-{
-	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
-	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
-	struct drm_connector_state *conn_state =
-		intel_dp->attached_connector->base.state;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
-	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
-
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
 		DRM_ERROR("Failed to get link status\n");
-		return;
+		return false;
 	}
 
-	if (!conn_state->crtc)
-		return;
-
-	WARN_ON(!drm_modeset_is_locked(&conn_state->crtc->mutex));
-
-	if (!conn_state->crtc->state->active)
-		return;
-
-	if (conn_state->commit &&
-	    !try_wait_for_completion(&conn_state->commit->hw_done))
-		return;
-
 	/*
 	 * Validate the cached values of intel_dp->link_rate and
 	 * intel_dp->lane_count before attempting to retrain.
 	 */
 	if (!intel_dp_link_params_valid(intel_dp, intel_dp->link_rate,
 					intel_dp->lane_count))
-		return;
+		return false;
 
 	/* Retrain if Channel EQ or CR not ok */
-	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
-		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
-			      intel_encoder->base.name);
+	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
+}
 
-		intel_dp_retrain_link(intel_dp);
+/*
+ * If display is now connected check links status,
+ * there has been known issues of link loss triggering
+ * long pulse.
+ *
+ * Some sinks (eg. ASUS PB287Q) seem to perform some
+ * weird HPD ping pong during modesets. So we can apparently
+ * end up with HPD going low during a modeset, and then
+ * going back up soon after. And once that happens we must
+ * retrain the link to get a picture. That's in case no
+ * userspace component reacted to intermittent HPD dip.
+ */
+int intel_dp_retrain_link(struct intel_encoder *encoder,
+			  struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	struct intel_connector *connector = intel_dp->attached_connector;
+	struct drm_connector_state *conn_state;
+	struct intel_crtc_state *crtc_state;
+	struct intel_crtc *crtc;
+	int ret;
+
+	/* FIXME handle the MST connectors as well */
+
+	if (!connector || connector->base.status != connector_status_connected)
+		return 0;
+
+	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, ctx);
+	if (ret)
+		return ret;
+
+	conn_state = connector->base.state;
+
+	crtc = to_intel_crtc(conn_state->crtc);
+	if (!crtc)
+		return 0;
+
+	ret = drm_modeset_lock(&crtc->base.mutex, ctx);
+	if (ret)
+		return ret;
+
+	crtc_state = to_intel_crtc_state(crtc->base.state);
+
+	WARN_ON(!intel_crtc_has_dp_encoder(crtc_state));
+
+	if (!crtc_state->base.active)
+		return 0;
+
+	if (conn_state->commit &&
+	    !try_wait_for_completion(&conn_state->commit->hw_done))
+		return 0;
+
+	if (!intel_dp_needs_link_retrain(intel_dp))
+		return 0;
+
+	/* Suppress underruns caused by re-training */
+	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
+	if (crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv,
+						      intel_crtc_pch_transcoder(crtc), false);
+
+	intel_dp_start_link_train(intel_dp);
+	intel_dp_stop_link_train(intel_dp);
+
+	/* Keep underrun reporting disabled until things are stable */
+	intel_wait_for_vblank(dev_priv, crtc->pipe);
+
+	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true);
+	if (crtc->config->has_pch_encoder)
+		intel_set_pch_fifo_underrun_reporting(dev_priv,
+						      intel_crtc_pch_transcoder(crtc), true);
+
+	return 0;
+}
+
+/*
+ * If display is now connected check links status,
+ * there has been known issues of link loss triggering
+ * long pulse.
+ *
+ * Some sinks (eg. ASUS PB287Q) seem to perform some
+ * weird HPD ping pong during modesets. So we can apparently
+ * end up with HPD going low during a modeset, and then
+ * going back up soon after. And once that happens we must
+ * retrain the link to get a picture. That's in case no
+ * userspace component reacted to intermittent HPD dip.
+ */
+static void intel_dp_post_hotplug(struct intel_encoder *encoder)
+{
+	struct drm_modeset_acquire_ctx ctx;
+	int ret;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	for (;;) {
+		ret = intel_dp_retrain_link(encoder, &ctx);
+
+		if (ret == -EDEADLK) {
+			drm_modeset_backoff(&ctx);
+			continue;
+		}
+
+		break;
 	}
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
 }
 
 /*
@@ -4376,7 +4439,9 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
-	intel_dp_check_link_status(intel_dp);
+	/* defer to the hotplug work for link retraining if needed */
+	if (intel_dp_needs_link_retrain(intel_dp))
+		return false;
 
 	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
 		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
@@ -4761,20 +4826,6 @@ intel_dp_long_pulse(struct intel_connector *connector)
 		 */
 		status = connector_status_disconnected;
 		goto out;
-	} else {
-		/*
-		 * If display is now connected check links status,
-		 * there has been known issues of link loss triggerring
-		 * long pulse.
-		 *
-		 * Some sinks (eg. ASUS PB287Q) seem to perform some
-		 * weird HPD ping pong during modesets. So we can apparently
-		 * end up with HPD going low during a modeset, and then
-		 * going back up soon after. And once that happens we must
-		 * retrain the link to get a picture. That's in case no
-		 * userspace component reacted to intermittent HPD dip.
-		 */
-		intel_dp_check_link_status(intel_dp);
 	}
 
 	/*
@@ -5119,37 +5170,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 	}
 
 	if (!intel_dp->is_mst) {
-		struct drm_modeset_acquire_ctx ctx;
-		struct drm_connector *connector = &intel_dp->attached_connector->base;
-		struct drm_crtc *crtc;
-		int iret;
-		bool handled = false;
-
-		drm_modeset_acquire_init(&ctx, 0);
-retry:
-		iret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
-		if (iret)
-			goto err;
-
-		crtc = connector->state->crtc;
-		if (crtc) {
-			iret = drm_modeset_lock(&crtc->mutex, &ctx);
-			if (iret)
-				goto err;
-		}
+		bool handled;
 
 		handled = intel_dp_short_pulse(intel_dp);
 
-err:
-		if (iret == -EDEADLK) {
-			drm_modeset_backoff(&ctx);
-			goto retry;
-		}
-
-		drm_modeset_drop_locks(&ctx);
-		drm_modeset_acquire_fini(&ctx);
-		WARN(iret, "Acquiring modeset locks failed with %i\n", iret);
-
 		if (!handled) {
 			intel_dp->detect_done = false;
 			goto put_power;
@@ -6170,6 +6194,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
 			     "DP %c", port_name(port)))
 		goto err_encoder_init;
 
+	intel_encoder->post_hotplug = intel_dp_post_hotplug;
 	intel_encoder->compute_config = intel_dp_compute_config;
 	intel_encoder->get_hw_state = intel_dp_get_hw_state;
 	intel_encoder->get_config = intel_dp_get_config;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4a7e603ccc38..f95ac56784e6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1530,6 +1530,8 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count);
 void intel_dp_start_link_train(struct intel_dp *intel_dp);
 void intel_dp_stop_link_train(struct intel_dp *intel_dp);
+int intel_dp_retrain_link(struct intel_encoder *encoder,
+			  struct drm_modeset_acquire_ctx *ctx);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
-- 
2.13.6

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

Re: [PATCH 3/3] drm/i915: Move SST DP link retraining into the ->post_hotplug() hook

[Manasi Navare]

On Fri, Dec 22, 2017 at 08:28:57PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Doing link reatrining from the short pulse handler is problematic since
> that might introduce deadlocks with MST sideband processing. Currently
> we don't retrain MST links from this code, but we want to change that.
> So better to move the entire thing to the hotplug work. We can utilize
> the new encoder->post_hotplug() hook for this.
> 
> The only thing we leave in the short pulse handler is the link status
> check. That one still depends on the link parameters stored under
> intel_dp, so no locking around that but races should be mostly harmless
> as the actual retraining code will recheck the link state if we
> end up there by mistake.
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |   7 +-
>  drivers/gpu/drm/i915/intel_dp.c  | 213 ++++++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/intel_drv.h |   2 +
>  3 files changed, 125 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 12da7024f01a..7e9964794efe 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2778,7 +2778,9 @@ static void intel_ddi_post_hotplug(struct intel_encoder *encoder)
>  	drm_modeset_acquire_init(&ctx, 0);
>  
>  	for (;;) {
> -		ret = intel_ddi_hdmi_reset_scrambling(encoder, &ctx);
> +		ret = intel_dp_retrain_link(encoder, &ctx);
> +		if (!ret)
> +			ret = intel_ddi_hdmi_reset_scrambling(encoder, &ctx);
>  
>  		if (ret == -EDEADLK) {
>  			drm_modeset_backoff(&ctx);
> @@ -2903,8 +2905,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>  	drm_encoder_init(&dev_priv->drm, encoder, &intel_ddi_funcs,
>  			 DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
>  
> -	if (init_hdmi)
> -		intel_encoder->post_hotplug = intel_ddi_post_hotplug;
> +	intel_encoder->post_hotplug = intel_ddi_post_hotplug;

Same goes here for the name, may be call it intel_ddi_hotplug_post_detect()?

>  	intel_encoder->compute_output_type = intel_ddi_compute_output_type;
>  	intel_encoder->compute_config = intel_ddi_compute_config;
>  	intel_encoder->enable = intel_enable_ddi;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 35c5299feab6..72234c5dc15b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4251,74 +4251,137 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  	return -EINVAL;
>  }
>  
> -static void
> -intel_dp_retrain_link(struct intel_dp *intel_dp)
> +static bool
> +intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
>  {
> -	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> -
> -	/* Suppress underruns caused by re-training */
> -	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
> -	if (crtc->config->has_pch_encoder)
> -		intel_set_pch_fifo_underrun_reporting(dev_priv,
> -						      intel_crtc_pch_transcoder(crtc), false);
> -
> -	intel_dp_start_link_train(intel_dp);
> -	intel_dp_stop_link_train(intel_dp);
> -
> -	/* Keep underrun reporting disabled until things are stable */
> -	intel_wait_for_vblank(dev_priv, crtc->pipe);
> -
> -	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true);
> -	if (crtc->config->has_pch_encoder)
> -		intel_set_pch_fifo_underrun_reporting(dev_priv,
> -						      intel_crtc_pch_transcoder(crtc), true);
> -}
> -
> -static void
> -intel_dp_check_link_status(struct intel_dp *intel_dp)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> -	struct drm_connector_state *conn_state =
> -		intel_dp->attached_connector->base.state;
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
> -	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
> -
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  		DRM_ERROR("Failed to get link status\n");
> -		return;
> +		return false;
>  	}
>  
> -	if (!conn_state->crtc)
> -		return;
> -
> -	WARN_ON(!drm_modeset_is_locked(&conn_state->crtc->mutex));
> -
> -	if (!conn_state->crtc->state->active)
> -		return;
> -
> -	if (conn_state->commit &&
> -	    !try_wait_for_completion(&conn_state->commit->hw_done))
> -		return;
> -
>  	/*
>  	 * Validate the cached values of intel_dp->link_rate and
>  	 * intel_dp->lane_count before attempting to retrain.
>  	 */
>  	if (!intel_dp_link_params_valid(intel_dp, intel_dp->link_rate,
>  					intel_dp->lane_count))
> -		return;
> +		return false;
>  
>  	/* Retrain if Channel EQ or CR not ok */
> -	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
> -		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> -			      intel_encoder->base.name);
> +	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> +}
>  
> -		intel_dp_retrain_link(intel_dp);
> +/*
> + * If display is now connected check links status,
> + * there has been known issues of link loss triggering
> + * long pulse.
> + *
> + * Some sinks (eg. ASUS PB287Q) seem to perform some
> + * weird HPD ping pong during modesets. So we can apparently
> + * end up with HPD going low during a modeset, and then
> + * going back up soon after. And once that happens we must
> + * retrain the link to get a picture. That's in case no
> + * userspace component reacted to intermittent HPD dip.
> + */
> +int intel_dp_retrain_link(struct intel_encoder *encoder,
> +			  struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +	struct drm_connector_state *conn_state;
> +	struct intel_crtc_state *crtc_state;
> +	struct intel_crtc *crtc;
> +	int ret;
> +
> +	/* FIXME handle the MST connectors as well */
> +
> +	if (!connector || connector->base.status != connector_status_connected)
> +		return 0;
> +
> +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, ctx);
> +	if (ret)
> +		return ret;
> +
> +	conn_state = connector->base.state;
> +
> +	crtc = to_intel_crtc(conn_state->crtc);
> +	if (!crtc)
> +		return 0;
> +
> +	ret = drm_modeset_lock(&crtc->base.mutex, ctx);
> +	if (ret)
> +		return ret;
> +
> +	crtc_state = to_intel_crtc_state(crtc->base.state);
> +
> +	WARN_ON(!intel_crtc_has_dp_encoder(crtc_state));
> +
> +	if (!crtc_state->base.active)
> +		return 0;
> +
> +	if (conn_state->commit &&
> +	    !try_wait_for_completion(&conn_state->commit->hw_done))
> +		return 0;
> +
> +	if (!intel_dp_needs_link_retrain(intel_dp))
> +		return 0;
> +
> +	/* Suppress underruns caused by re-training */
> +	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
> +	if (crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv,
> +						      intel_crtc_pch_transcoder(crtc), false);
> +
> +	intel_dp_start_link_train(intel_dp);
> +	intel_dp_stop_link_train(intel_dp);
> +
> +	/* Keep underrun reporting disabled until things are stable */
> +	intel_wait_for_vblank(dev_priv, crtc->pipe);
> +
> +	intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, true);
> +	if (crtc->config->has_pch_encoder)
> +		intel_set_pch_fifo_underrun_reporting(dev_priv,
> +						      intel_crtc_pch_transcoder(crtc), true);
> +
> +	return 0;
> +}
> +
> +/*
> + * If display is now connected check links status,
> + * there has been known issues of link loss triggering
> + * long pulse.
> + *
> + * Some sinks (eg. ASUS PB287Q) seem to perform some
> + * weird HPD ping pong during modesets. So we can apparently
> + * end up with HPD going low during a modeset, and then
> + * going back up soon after. And once that happens we must
> + * retrain the link to get a picture. That's in case no
> + * userspace component reacted to intermittent HPD dip.
> + */
> +static void intel_dp_post_hotplug(struct intel_encoder *encoder)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	int ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	for (;;) {
> +		ret = intel_dp_retrain_link(encoder, &ctx);
> +
> +		if (ret == -EDEADLK) {
> +			drm_modeset_backoff(&ctx);
> +			continue;
> +		}
> +
> +		break;
>  	}
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +	WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>  }
>  

I like that now intel_dp_long_pulse() and intel_dp_short_pulse() dont need to call
link retraining separately. This post_hotplug will take care of retraining after long and short
pulse.

Acked-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

>  /*
> @@ -4376,7 +4439,9 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>  	}
>  
> -	intel_dp_check_link_status(intel_dp);
> +	/* defer to the hotplug work for link retraining if needed */
> +	if (intel_dp_needs_link_retrain(intel_dp))
> +		return false;
>  
>  	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
>  		DRM_DEBUG_KMS("Link Training Compliance Test requested\n");
> @@ -4761,20 +4826,6 @@ intel_dp_long_pulse(struct intel_connector *connector)
>  		 */
>  		status = connector_status_disconnected;
>  		goto out;
> -	} else {
> -		/*
> -		 * If display is now connected check links status,
> -		 * there has been known issues of link loss triggerring
> -		 * long pulse.
> -		 *
> -		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> -		 * weird HPD ping pong during modesets. So we can apparently
> -		 * end up with HPD going low during a modeset, and then
> -		 * going back up soon after. And once that happens we must
> -		 * retrain the link to get a picture. That's in case no
> -		 * userspace component reacted to intermittent HPD dip.
> -		 */
> -		intel_dp_check_link_status(intel_dp);
>  	}
>  
>  	/*
> @@ -5119,37 +5170,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  	}
>  
>  	if (!intel_dp->is_mst) {
> -		struct drm_modeset_acquire_ctx ctx;
> -		struct drm_connector *connector = &intel_dp->attached_connector->base;
> -		struct drm_crtc *crtc;
> -		int iret;
> -		bool handled = false;
> -
> -		drm_modeset_acquire_init(&ctx, 0);
> -retry:
> -		iret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> -		if (iret)
> -			goto err;
> -
> -		crtc = connector->state->crtc;
> -		if (crtc) {
> -			iret = drm_modeset_lock(&crtc->mutex, &ctx);
> -			if (iret)
> -				goto err;
> -		}
> +		bool handled;
>  
>  		handled = intel_dp_short_pulse(intel_dp);
>  
> -err:
> -		if (iret == -EDEADLK) {
> -			drm_modeset_backoff(&ctx);
> -			goto retry;
> -		}
> -
> -		drm_modeset_drop_locks(&ctx);
> -		drm_modeset_acquire_fini(&ctx);
> -		WARN(iret, "Acquiring modeset locks failed with %i\n", iret);
> -
>  		if (!handled) {
>  			intel_dp->detect_done = false;
>  			goto put_power;
> @@ -6170,6 +6194,7 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
>  			     "DP %c", port_name(port)))
>  		goto err_encoder_init;
>  
> +	intel_encoder->post_hotplug = intel_dp_post_hotplug;
>  	intel_encoder->compute_config = intel_dp_compute_config;
>  	intel_encoder->get_hw_state = intel_dp_get_hw_state;
>  	intel_encoder->get_config = intel_dp_get_config;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4a7e603ccc38..f95ac56784e6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1530,6 +1530,8 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
>  					    int link_rate, uint8_t lane_count);
>  void intel_dp_start_link_train(struct intel_dp *intel_dp);
>  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +int intel_dp_retrain_link(struct intel_encoder *encoder,
> +			  struct drm_modeset_acquire_ctx *ctx);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>  void intel_dp_encoder_reset(struct drm_encoder *encoder);
>  void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
> -- 
> 2.13.6
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915: Split encoder->hot_plug() into pre and post variants

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Split encoder->hot_plug() into pre and post variants
URL   : https://patchwork.freedesktop.org/series/35732/
State : failure

== Summary ==

Series 35732v1 series starting with [1/3] drm/i915: Split encoder->hot_plug() into pre and post variants
https://patchwork.freedesktop.org/api/1.0/series/35732/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-warn -> DMESG-FAIL (fi-elk-e7500) fdo#103989
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                pass       -> FAIL       (fi-skl-6770hq)
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-skl-6770hq)
        Subgroup basic-flip-after-cursor-atomic:
                pass       -> FAIL       (fi-skl-6770hq)
        Subgroup basic-flip-after-cursor-legacy:
                pass       -> FAIL       (fi-skl-6770hq)
        Subgroup basic-flip-after-cursor-varying-size:
                pass       -> FAIL       (fi-skl-6770hq)
        Subgroup basic-flip-before-cursor-atomic:
                pass       -> FAIL       (fi-skl-6770hq)
        Subgroup basic-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-skl-6770hq)
        Subgroup basic-flip-before-cursor-varying-size:
                pass       -> FAIL       (fi-skl-6770hq)

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:436s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:443s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:383s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:493s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:275s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:497s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:493s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:477s
fi-elk-e7500     total:224  pass:163  dwarn:14  dfail:1   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:261s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:534s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:406s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:412s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:425s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:476s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:427s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:479s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:526s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:465s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:522s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:578s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:441s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:532s
fi-skl-6700hq    total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:555s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:506s
fi-skl-6770hq    total:288  pass:260  dwarn:0   dfail:0   fail:8   skip:20  time:508s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:437s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:544s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:420s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:591s
fi-cnl-y         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:625s
fi-byt-n2820 failed to collect. IGT log at Patchwork_7572/fi-byt-n2820/igt.log
fi-glk-dsi failed to collect. IGT log at Patchwork_7572/fi-glk-dsi/igt.log

b58679136779d486648809274b64d89fdafc79e7 drm-tip: 2017y-12m-22d-19h-04m-50s UTC integration manifest
b23451fe8d0f drm/i915: Move SST DP link retraining into the ->post_hotplug() hook
3f84c2d1f724 drm/i915: Reinitialize sink scrambling/TMDS clock ratio on HPD
460f4e0da62e drm/i915: Split encoder->hot_plug() into pre and post variants

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7572/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Split encoder->hot_plug() into pre and post variants

[Manasi Navare]

On Fri, Dec 22, 2017 at 08:28:55PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> During hpd processing we may want to do things both before and
> after the display detection. To that end split the encoder->hot_plug()
> hooks.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h     | 3 ++-
>  drivers/gpu/drm/i915/intel_hotplug.c | 6 ++++--
>  drivers/gpu/drm/i915/intel_sdvo.c    | 2 +-
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 30f791f89d64..4a7e603ccc38 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -211,7 +211,8 @@ struct intel_encoder {
>  	enum intel_output_type type;
>  	enum port port;
>  	unsigned int cloneable;
> -	void (*hot_plug)(struct intel_encoder *);
> +	void (*pre_hotplug)(struct intel_encoder *encoder);
> +	void (*post_hotplug)(struct intel_encoder *encoder);

Splitting into pre and post detect for the hotplug makes sense.
Although to be more intuitive of the order of things that happen on hotplug
the names could be hotplug_pre_detect() and hotplug_post_detect() or something
on those lines relative to the detect() call.
 Looks good other than that.

Manasi

>  	enum intel_output_type (*compute_output_type)(struct intel_encoder *,
>  						      struct intel_crtc_state *,
>  						      struct drm_connector_state *);
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index 875d5d218d5c..91abca3ae11a 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -370,10 +370,12 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
>  			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
>  				      connector->name, intel_encoder->hpd_pin);
> -			if (intel_encoder->hot_plug)
> -				intel_encoder->hot_plug(intel_encoder);
> +			if (intel_encoder->pre_hotplug)
> +				intel_encoder->pre_hotplug(intel_encoder);
>  			if (intel_hpd_irq_event(dev, connector))
>  				changed = true;
> +			if (intel_encoder->post_hotplug)
> +				intel_encoder->post_hotplug(intel_encoder);
>  		}
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 2b8764897d68..6de90a1fc553 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2496,7 +2496,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
>  		/* Some SDVO devices have one-shot hotplug interrupts.
>  		 * Ensure that they get re-enabled when an interrupt happens.
>  		 */
> -		intel_encoder->hot_plug = intel_sdvo_enable_hotplug;
> +		intel_encoder->pre_hotplug = intel_sdvo_enable_hotplug;
>  		intel_sdvo_enable_hotplug(intel_encoder);
>  	} else {
>  		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
> -- 
> 2.13.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx