Async eDP init

Async eDP init

[Jesse Barnes]

This updates my old patch for this, but w/o fixing the locking issue
Ville mentioned.  In looking at it, it seems like the sync point should
be at a higher level, maybe at the level of the atomic mode setting async
serialization points?  Another possibility would be to make it a lazy
init type function, sprinkled about but only running once when we first
need it.

Any thoughts from anyone?  I don't think I can just do a lock drop here,
since other threads may jump in and mess with underlying state.  That
shouldn't affect the eDP state we fill out, but may affect the state the
caller depended on in the first place...

Thanks,
Jesse

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

[PATCH] drm/i915/dp: move edp init to work queue

[Jesse Barnes]

This helps speed up driver init time, and puts off the eDP stuff until
we actually need it.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c  | 103 ++++++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h |   3 ++
 2 files changed, 73 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5256c06..2abd339 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3768,6 +3768,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 
 	DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd);
 
+	if (intel_dp->dpcd_valid)
+		return true;
+
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
 		return false; /* DPCD not present */
 
@@ -4012,6 +4015,25 @@ go_again:
 	return -EINVAL;
 }
 
+static void intel_flush_edp_cache_work(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp->attached_connector->base.dev;
+
+	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+
+	if (!is_edp(intel_dp))
+		return;
+
+	/*
+	 * FIXME: we need to synchronize this at a higher level, like the
+	 * first mode set or other display I/O activity.  Maybe re-use
+	 * async mode setting entry points?
+	 */
+	mutex_unlock(&dev->mode_config.mutex);
+	flush_work(&intel_dp->edp_cache_work);
+	mutex_lock(&dev->mode_config.mutex);
+}
+
 /*
  * According to DP spec
  * 5.1.2:
@@ -4044,6 +4066,8 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 	}
 
+	intel_flush_edp_cache_work(intel_dp);
+
 	/* Now read the DPCD to see if it's actually running */
 	if (!intel_dp_get_dpcd(intel_dp)) {
 		return;
@@ -4079,6 +4103,8 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	uint8_t *dpcd = intel_dp->dpcd;
 	uint8_t type;
 
+	intel_flush_edp_cache_work(intel_dp);
+
 	if (!intel_dp_get_dpcd(intel_dp))
 		return connector_status_disconnected;
 
@@ -4215,13 +4241,23 @@ g4x_dp_detect(struct intel_dp *intel_dp)
 	return intel_dp_detect_dpcd(intel_dp);
 }
 
+static bool intel_connector_has_edid(struct intel_connector *intel_connector)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base);
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+
+	intel_flush_edp_cache_work(intel_dp);
+
+	return intel_connector->edid != NULL;
+}
+
 static struct edid *
 intel_dp_get_edid(struct intel_dp *intel_dp)
 {
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 
 	/* use cached edid if we have one */
-	if (intel_connector->edid) {
+	if (intel_connector_has_edid(intel_connector)) {
 		/* invalid edid */
 		if (IS_ERR(intel_connector->edid))
 			return NULL;
@@ -4516,6 +4552,7 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	intel_dp_mst_encoder_cleanup(intel_dig_port);
 	if (is_edp(intel_dp)) {
 		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
+		cancel_work_sync(&intel_dp->edp_cache_work);
 		/*
 		 * vdd might still be enabled do to the delayed vdd off.
 		 * Make sure vdd is actually turned off here.
@@ -5316,9 +5353,11 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,
 	return downclock_mode;
 }
 
-static bool intel_edp_init_connector(struct intel_dp *intel_dp,
-				     struct intel_connector *intel_connector)
+static void intel_edp_cache_work(struct work_struct *work)
 {
+	struct intel_dp *intel_dp = container_of(work, struct intel_dp,
+						 edp_cache_work);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct drm_connector *connector = &intel_connector->base;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
@@ -5329,12 +5368,10 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	bool has_dpcd;
 	struct drm_display_mode *scan;
 	struct edid *edid;
-	enum pipe pipe = INVALID_PIPE;
 
 	dev_priv->drrs.type = DRRS_NOT_SUPPORTED;
 
-	if (!is_edp(intel_dp))
-		return true;
+	mutex_lock(&dev->mode_config.mutex);
 
 	pps_lock(intel_dp);
 	intel_edp_panel_vdd_sanitize(intel_dp);
@@ -5348,10 +5385,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 			dev_priv->no_aux_handshake =
 				intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
 				DP_NO_AUX_HANDSHAKE_LINK_TRAINING;
+		intel_dp->dpcd_valid = true;
 	} else {
-		/* if this fails, presume the device is a ghost */
-		DRM_INFO("failed to retrieve link info, disabling eDP\n");
-		return false;
+		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
+		edp_panel_vdd_off_sync(intel_dp);
+		drm_connector_cleanup(connector);
+		mutex_unlock(&dev->mode_config.mutex);
+		return;
 	}
 
 	/* We now know it's not a ghost, init power sequence regs. */
@@ -5359,7 +5399,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
 	pps_unlock(intel_dp);
 
-	mutex_lock(&dev->mode_config.mutex);
 	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
 	if (edid) {
 		if (drm_add_edid_modes(connector, edid)) {
@@ -5384,7 +5423,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 			break;
 		}
 	}
-
 	/* fallback to VBT if available for eDP */
 	if (!fixed_mode && dev_priv->vbt.lfp_lvds_vbt_mode) {
 		fixed_mode = drm_mode_duplicate(dev,
@@ -5392,7 +5430,26 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 		if (fixed_mode)
 			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
 	}
+
+	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
+	intel_connector->panel.backlight_power = intel_edp_backlight_power;
+
 	mutex_unlock(&dev->mode_config.mutex);
+}
+
+static void intel_edp_init_connector(struct intel_dp *intel_dp,
+				     struct intel_connector *intel_connector)
+{
+	struct drm_connector *connector = &intel_connector->base;
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	enum pipe pipe = INVALID_PIPE;
+
+	if (!is_edp(intel_dp))
+		return;
+
+	INIT_WORK(&intel_dp->edp_cache_work, intel_edp_cache_work);
+
+	schedule_work(&intel_dp->edp_cache_work);
 
 	if (IS_VALLEYVIEW(dev)) {
 		intel_dp->edp_notifier.notifier_call = edp_notify_handler;
@@ -5418,11 +5475,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 			      pipe_name(pipe));
 	}
 
-	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
-	intel_connector->panel.backlight_power = intel_edp_backlight_power;
 	intel_panel_setup_backlight(connector, pipe);
-
-	return true;
 }
 
 bool
@@ -5488,8 +5541,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	connector->interlace_allowed = true;
 	connector->doublescan_allowed = 0;
 
-	INIT_DELAYED_WORK(&intel_dp->panel_vdd_work,
-			  edp_panel_vdd_work);
+	INIT_DELAYED_WORK(&intel_dp->panel_vdd_work, edp_panel_vdd_work);
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
 	drm_connector_register(connector);
@@ -5538,22 +5590,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		}
 	}
 
-	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
-		drm_dp_aux_unregister(&intel_dp->aux);
-		if (is_edp(intel_dp)) {
-			cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
-			/*
-			 * vdd might still be enabled do to the delayed vdd off.
-			 * Make sure vdd is actually turned off here.
-			 */
-			pps_lock(intel_dp);
-			edp_panel_vdd_off_sync(intel_dp);
-			pps_unlock(intel_dp);
-		}
-		drm_connector_unregister(connector);
-		drm_connector_cleanup(connector);
-		return false;
-	}
+	intel_edp_init_connector(intel_dp, intel_connector);
 
 	intel_dp_add_properties(intel_dp, connector);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a1baaa1..8cee4a2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -656,12 +656,15 @@ struct intel_dp {
 	bool can_mst; /* this port supports mst */
 	bool is_mst;
 	int active_mst_links;
+	bool dpcd_valid; /* for eDP DPCD caching */
 	/* connector directly attached - won't be use for modeset in mst world */
 	struct intel_connector *attached_connector;
 
 	/* mst connector list */
 	struct intel_dp_mst_encoder *mst_encoders[I915_MAX_PIPES];
 	struct drm_dp_mst_topology_mgr mst_mgr;
+	struct work_struct edp_cache_work;
+	const char *i2c_name;
 
 	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
 	/*
-- 
1.9.1

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

Re: [PATCH] drm/i915/dp: move edp init to work queue

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5997
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -2              272/272              270/272
ILK                                  301/301              301/301
SNB                                  303/303              303/303
IVB                                  342/342              342/342
BYT                                  287/287              287/287
HSW                                  362/362              362/362
BDW                                  308/308              308/308
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gem_userptr_blits_minor-unsync-interruptible      PASS(1)      DMESG_WARN(1)PASS(1)
*PNV  igt_gem_userptr_blits_minor-unsync-normal      PASS(1)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Async eDP init

[Daniel Vetter]

On Wed, Mar 18, 2015 at 11:41:48AM -0700, Jesse Barnes wrote:
> This updates my old patch for this, but w/o fixing the locking issue
> Ville mentioned.  In looking at it, it seems like the sync point should
> be at a higher level, maybe at the level of the atomic mode setting async
> serialization points?  Another possibility would be to make it a lazy
> init type function, sprinkled about but only running once when we first
> need it.
> 
> Any thoughts from anyone?  I don't think I can just do a lock drop here,
> since other threads may jump in and mess with underlying state.  That
> shouldn't affect the eDP state we fill out, but may affect the state the
> caller depended on in the first place...

Imo the real issue is that we register a connector and then throw it away
again. Not that big a problem any more since mst dp happened meanwhile but
still might result in confusion.

I think we should try to at least get the "is this an edp or not" question
right, and only postpone the other init steps. So maybe start with making
that edp failed to init issue really loud and then rip it out?

Postponing all the other init work would be comparitively a lot easier I
think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Async eDP init

[Jesse Barnes]

On 03/19/2015 10:42 AM, Daniel Vetter wrote:
> On Wed, Mar 18, 2015 at 11:41:48AM -0700, Jesse Barnes wrote:
>> This updates my old patch for this, but w/o fixing the locking issue
>> Ville mentioned.  In looking at it, it seems like the sync point should
>> be at a higher level, maybe at the level of the atomic mode setting async
>> serialization points?  Another possibility would be to make it a lazy
>> init type function, sprinkled about but only running once when we first
>> need it.
>>
>> Any thoughts from anyone?  I don't think I can just do a lock drop here,
>> since other threads may jump in and mess with underlying state.  That
>> shouldn't affect the eDP state we fill out, but may affect the state the
>> caller depended on in the first place...
> 
> Imo the real issue is that we register a connector and then throw it away
> again. Not that big a problem any more since mst dp happened meanwhile but
> still might result in confusion.
> 
> I think we should try to at least get the "is this an edp or not" question
> right, and only postpone the other init steps. So maybe start with making
> that edp failed to init issue really loud and then rip it out?
> 
> Postponing all the other init work would be comparitively a lot easier I
> think.

I didn't view that as a big issue, but it should be easy to solve.  I
think the synchronization problems are still just as thorny though, even
with the question of is_edp() solved early.  The eDP init is kind of
like a boot time mode set, but one that needs to complete before any
activity on the port.

I'll check those init paths; hopefully answering is_edp() won't have a
bunch of delay in itself.

Jesse

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

Re: Async eDP init

[Jesse Barnes]

On 03/19/2015 11:00 AM, Jesse Barnes wrote:
> On 03/19/2015 10:42 AM, Daniel Vetter wrote:
>> On Wed, Mar 18, 2015 at 11:41:48AM -0700, Jesse Barnes wrote:
>>> This updates my old patch for this, but w/o fixing the locking issue
>>> Ville mentioned.  In looking at it, it seems like the sync point should
>>> be at a higher level, maybe at the level of the atomic mode setting async
>>> serialization points?  Another possibility would be to make it a lazy
>>> init type function, sprinkled about but only running once when we first
>>> need it.
>>>
>>> Any thoughts from anyone?  I don't think I can just do a lock drop here,
>>> since other threads may jump in and mess with underlying state.  That
>>> shouldn't affect the eDP state we fill out, but may affect the state the
>>> caller depended on in the first place...
>>
>> Imo the real issue is that we register a connector and then throw it away
>> again. Not that big a problem any more since mst dp happened meanwhile but
>> still might result in confusion.
>>
>> I think we should try to at least get the "is this an edp or not" question
>> right, and only postpone the other init steps. So maybe start with making
>> that edp failed to init issue really loud and then rip it out?
>>
>> Postponing all the other init work would be comparitively a lot easier I
>> think.
> 
> I didn't view that as a big issue, but it should be easy to solve.  I
> think the synchronization problems are still just as thorny though, even
> with the question of is_edp() solved early.  The eDP init is kind of
> like a boot time mode set, but one that needs to complete before any
> activity on the port.
> 
> I'll check those init paths; hopefully answering is_edp() won't have a
> bunch of delay in itself.

So the answer is unfortunately no.  The DPCD read we do at the top of
the eDP init function is the one we use to check whether the port should
exist, and it's the function that takes a long time (~700ms on this
machine).

Jesse

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

Re: Async eDP init

[Ville Syrjälä]

On Thu, Mar 19, 2015 at 11:40:15AM -0700, Jesse Barnes wrote:
> On 03/19/2015 11:00 AM, Jesse Barnes wrote:
> > On 03/19/2015 10:42 AM, Daniel Vetter wrote:
> >> On Wed, Mar 18, 2015 at 11:41:48AM -0700, Jesse Barnes wrote:
> >>> This updates my old patch for this, but w/o fixing the locking issue
> >>> Ville mentioned.  In looking at it, it seems like the sync point should
> >>> be at a higher level, maybe at the level of the atomic mode setting async
> >>> serialization points?  Another possibility would be to make it a lazy
> >>> init type function, sprinkled about but only running once when we first
> >>> need it.
> >>>
> >>> Any thoughts from anyone?  I don't think I can just do a lock drop here,
> >>> since other threads may jump in and mess with underlying state.  That
> >>> shouldn't affect the eDP state we fill out, but may affect the state the
> >>> caller depended on in the first place...
> >>
> >> Imo the real issue is that we register a connector and then throw it away
> >> again. Not that big a problem any more since mst dp happened meanwhile but
> >> still might result in confusion.
> >>
> >> I think we should try to at least get the "is this an edp or not" question
> >> right, and only postpone the other init steps. So maybe start with making
> >> that edp failed to init issue really loud and then rip it out?
> >>
> >> Postponing all the other init work would be comparitively a lot easier I
> >> think.
> > 
> > I didn't view that as a big issue, but it should be easy to solve.  I
> > think the synchronization problems are still just as thorny though, even
> > with the question of is_edp() solved early.  The eDP init is kind of
> > like a boot time mode set, but one that needs to complete before any
> > activity on the port.
> > 
> > I'll check those init paths; hopefully answering is_edp() won't have a
> > bunch of delay in itself.
> 
> So the answer is unfortunately no.  The DPCD read we do at the top of
> the eDP init function is the one we use to check whether the port should
> exist, and it's the function that takes a long time (~700ms on this
> machine).

Why is it taking that long? Even with an external display connected my
BSW boots with VDD forced on, so in theory it should just go read the
DPCD without any power sequencing needed, unless the delayed vdd off
work somehow manages to execute between the intel_edp_panel_vdd_sanitize()
call and intel_dp_get_dpcd() call...

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

Re: Async eDP init

[Jesse Barnes]

On 03/19/2015 11:53 AM, Ville Syrjälä wrote:
> On Thu, Mar 19, 2015 at 11:40:15AM -0700, Jesse Barnes wrote:
>> On 03/19/2015 11:00 AM, Jesse Barnes wrote:
>>> On 03/19/2015 10:42 AM, Daniel Vetter wrote:
>>>> On Wed, Mar 18, 2015 at 11:41:48AM -0700, Jesse Barnes wrote:
>>>>> This updates my old patch for this, but w/o fixing the locking issue
>>>>> Ville mentioned.  In looking at it, it seems like the sync point should
>>>>> be at a higher level, maybe at the level of the atomic mode setting async
>>>>> serialization points?  Another possibility would be to make it a lazy
>>>>> init type function, sprinkled about but only running once when we first
>>>>> need it.
>>>>>
>>>>> Any thoughts from anyone?  I don't think I can just do a lock drop here,
>>>>> since other threads may jump in and mess with underlying state.  That
>>>>> shouldn't affect the eDP state we fill out, but may affect the state the
>>>>> caller depended on in the first place...
>>>>
>>>> Imo the real issue is that we register a connector and then throw it away
>>>> again. Not that big a problem any more since mst dp happened meanwhile but
>>>> still might result in confusion.
>>>>
>>>> I think we should try to at least get the "is this an edp or not" question
>>>> right, and only postpone the other init steps. So maybe start with making
>>>> that edp failed to init issue really loud and then rip it out?
>>>>
>>>> Postponing all the other init work would be comparitively a lot easier I
>>>> think.
>>>
>>> I didn't view that as a big issue, but it should be easy to solve.  I
>>> think the synchronization problems are still just as thorny though, even
>>> with the question of is_edp() solved early.  The eDP init is kind of
>>> like a boot time mode set, but one that needs to complete before any
>>> activity on the port.
>>>
>>> I'll check those init paths; hopefully answering is_edp() won't have a
>>> bunch of delay in itself.
>>
>> So the answer is unfortunately no.  The DPCD read we do at the top of
>> the eDP init function is the one we use to check whether the port should
>> exist, and it's the function that takes a long time (~700ms on this
>> machine).
> 
> Why is it taking that long? Even with an external display connected my
> BSW boots with VDD forced on, so in theory it should just go read the
> DPCD without any power sequencing needed, unless the delayed vdd off
> work somehow manages to execute between the intel_edp_panel_vdd_sanitize()
> call and intel_dp_get_dpcd() call...

Yeah, in my config somehow the DPCD read does end up incurring the cost
of the PPS.  The panel is on when the kernel loads too.  It seems
unlikely that the vdd_sanitize runs first, but I suppose it's possible
if the delay is 0?  Hm no that doesn't seem to be happening here.

Looks like we're doing the panel power sequence delays due to the panel
not being on:

...	if (!edp_have_panel_power(intel_dp))
		wait_panel_power_cycle(intel_dp);
...
	if (!edp_have_panel_power(intel_dp)) {
		DRM_DEBUG_KMS("eDP port %c panel power wasn't enabled\n",
			      port_name(intel_dig_port->port));
		msleep(intel_dp->panel_power_up_delay);
	}
...


Do you not see that on your machine?

Jesse

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

Re: Async eDP init

[Ville Syrjälä]

On Thu, Mar 19, 2015 at 12:13:24PM -0700, Jesse Barnes wrote:
> On 03/19/2015 11:53 AM, Ville Syrjälä wrote:
> > On Thu, Mar 19, 2015 at 11:40:15AM -0700, Jesse Barnes wrote:
> >> On 03/19/2015 11:00 AM, Jesse Barnes wrote:
> >>> On 03/19/2015 10:42 AM, Daniel Vetter wrote:
> >>>> On Wed, Mar 18, 2015 at 11:41:48AM -0700, Jesse Barnes wrote:
> >>>>> This updates my old patch for this, but w/o fixing the locking issue
> >>>>> Ville mentioned.  In looking at it, it seems like the sync point should
> >>>>> be at a higher level, maybe at the level of the atomic mode setting async
> >>>>> serialization points?  Another possibility would be to make it a lazy
> >>>>> init type function, sprinkled about but only running once when we first
> >>>>> need it.
> >>>>>
> >>>>> Any thoughts from anyone?  I don't think I can just do a lock drop here,
> >>>>> since other threads may jump in and mess with underlying state.  That
> >>>>> shouldn't affect the eDP state we fill out, but may affect the state the
> >>>>> caller depended on in the first place...
> >>>>
> >>>> Imo the real issue is that we register a connector and then throw it away
> >>>> again. Not that big a problem any more since mst dp happened meanwhile but
> >>>> still might result in confusion.
> >>>>
> >>>> I think we should try to at least get the "is this an edp or not" question
> >>>> right, and only postpone the other init steps. So maybe start with making
> >>>> that edp failed to init issue really loud and then rip it out?
> >>>>
> >>>> Postponing all the other init work would be comparitively a lot easier I
> >>>> think.
> >>>
> >>> I didn't view that as a big issue, but it should be easy to solve.  I
> >>> think the synchronization problems are still just as thorny though, even
> >>> with the question of is_edp() solved early.  The eDP init is kind of
> >>> like a boot time mode set, but one that needs to complete before any
> >>> activity on the port.
> >>>
> >>> I'll check those init paths; hopefully answering is_edp() won't have a
> >>> bunch of delay in itself.
> >>
> >> So the answer is unfortunately no.  The DPCD read we do at the top of
> >> the eDP init function is the one we use to check whether the port should
> >> exist, and it's the function that takes a long time (~700ms on this
> >> machine).
> > 
> > Why is it taking that long? Even with an external display connected my
> > BSW boots with VDD forced on, so in theory it should just go read the
> > DPCD without any power sequencing needed, unless the delayed vdd off
> > work somehow manages to execute between the intel_edp_panel_vdd_sanitize()
> > call and intel_dp_get_dpcd() call...
> 
> Yeah, in my config somehow the DPCD read does end up incurring the cost
> of the PPS.  The panel is on when the kernel loads too.  It seems
> unlikely that the vdd_sanitize runs first, but I suppose it's possible
> if the delay is 0?  Hm no that doesn't seem to be happening here.
> 
> Looks like we're doing the panel power sequence delays due to the panel
> not being on:
> 
> ...	if (!edp_have_panel_power(intel_dp))
> 		wait_panel_power_cycle(intel_dp);
> ...
> 	if (!edp_have_panel_power(intel_dp)) {
> 		DRM_DEBUG_KMS("eDP port %c panel power wasn't enabled\n",
> 			      port_name(intel_dig_port->port));
> 		msleep(intel_dp->panel_power_up_delay);
> 	}
> ...

Hmm. The edp_have_panel_vdd() check should have returned already earlier
assuming VDD is still on at this point.

> Do you not see that on your machine?

Not sure. Imre borrowed my machine to fix some bugs while I'm busy with
other stuff so can't check now. But I can check tomorrow.

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

Re: Async eDP init

[Daniel Vetter]

On Thu, Mar 19, 2015 at 12:13:24PM -0700, Jesse Barnes wrote:
> On 03/19/2015 11:53 AM, Ville Syrjälä wrote:
> > On Thu, Mar 19, 2015 at 11:40:15AM -0700, Jesse Barnes wrote:
> >> On 03/19/2015 11:00 AM, Jesse Barnes wrote:
> >>> On 03/19/2015 10:42 AM, Daniel Vetter wrote:
> >>>> On Wed, Mar 18, 2015 at 11:41:48AM -0700, Jesse Barnes wrote:
> >>>>> This updates my old patch for this, but w/o fixing the locking issue
> >>>>> Ville mentioned.  In looking at it, it seems like the sync point should
> >>>>> be at a higher level, maybe at the level of the atomic mode setting async
> >>>>> serialization points?  Another possibility would be to make it a lazy
> >>>>> init type function, sprinkled about but only running once when we first
> >>>>> need it.
> >>>>>
> >>>>> Any thoughts from anyone?  I don't think I can just do a lock drop here,
> >>>>> since other threads may jump in and mess with underlying state.  That
> >>>>> shouldn't affect the eDP state we fill out, but may affect the state the
> >>>>> caller depended on in the first place...
> >>>>
> >>>> Imo the real issue is that we register a connector and then throw it away
> >>>> again. Not that big a problem any more since mst dp happened meanwhile but
> >>>> still might result in confusion.
> >>>>
> >>>> I think we should try to at least get the "is this an edp or not" question
> >>>> right, and only postpone the other init steps. So maybe start with making
> >>>> that edp failed to init issue really loud and then rip it out?
> >>>>
> >>>> Postponing all the other init work would be comparitively a lot easier I
> >>>> think.
> >>>
> >>> I didn't view that as a big issue, but it should be easy to solve.  I
> >>> think the synchronization problems are still just as thorny though, even
> >>> with the question of is_edp() solved early.  The eDP init is kind of
> >>> like a boot time mode set, but one that needs to complete before any
> >>> activity on the port.
> >>>
> >>> I'll check those init paths; hopefully answering is_edp() won't have a
> >>> bunch of delay in itself.
> >>
> >> So the answer is unfortunately no.  The DPCD read we do at the top of
> >> the eDP init function is the one we use to check whether the port should
> >> exist, and it's the function that takes a long time (~700ms on this
> >> machine).
> > 
> > Why is it taking that long? Even with an external display connected my
> > BSW boots with VDD forced on, so in theory it should just go read the
> > DPCD without any power sequencing needed, unless the delayed vdd off
> > work somehow manages to execute between the intel_edp_panel_vdd_sanitize()
> > call and intel_dp_get_dpcd() call...
> 
> Yeah, in my config somehow the DPCD read does end up incurring the cost
> of the PPS.  The panel is on when the kernel loads too.  It seems
> unlikely that the vdd_sanitize runs first, but I suppose it's possible
> if the delay is 0?  Hm no that doesn't seem to be happening here.
> 
> Looks like we're doing the panel power sequence delays due to the panel
> not being on:
> 
> ...	if (!edp_have_panel_power(intel_dp))
> 		wait_panel_power_cycle(intel_dp);
> ...
> 	if (!edp_have_panel_power(intel_dp)) {
> 		DRM_DEBUG_KMS("eDP port %c panel power wasn't enabled\n",
> 			      port_name(intel_dig_port->port));
> 		msleep(intel_dp->panel_power_up_delay);
> 	}
> ...
> 
> 
> Do you not see that on your machine?

Hm, could it be that the initial pps_pipe selection somehow doesn't
understand the situation correctly? If the panel is powered up we really
should be able to avoid delays ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Async eDP init

[Daniel Vetter]

On Wed, Mar 18, 2015 at 11:41:48AM -0700, Jesse Barnes wrote:
> This updates my old patch for this, but w/o fixing the locking issue
> Ville mentioned.  In looking at it, it seems like the sync point should
> be at a higher level, maybe at the level of the atomic mode setting async
> serialization points?  Another possibility would be to make it a lazy
> init type function, sprinkled about but only running once when we first
> need it.
> 
> Any thoughts from anyone?  I don't think I can just do a lock drop here,
> since other threads may jump in and mess with underlying state.  That
> shouldn't affect the eDP state we fill out, but may affect the state the
> caller depended on in the first place...

Also, has boot-time actually increased or did we simply push it somewhere
we don't measure the delay anymore? After all right afterwards we'll do
the fbcon setup, and that will synchronize everything again.

And on modern systems without fbcon I expect userspace to go around and do
a probe, which again would force synchronization pretty quickly ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Async eDP init

[Jesse Barnes]

On 03/19/2015 10:44 AM, Daniel Vetter wrote:
> On Wed, Mar 18, 2015 at 11:41:48AM -0700, Jesse Barnes wrote:
>> This updates my old patch for this, but w/o fixing the locking issue
>> Ville mentioned.  In looking at it, it seems like the sync point should
>> be at a higher level, maybe at the level of the atomic mode setting async
>> serialization points?  Another possibility would be to make it a lazy
>> init type function, sprinkled about but only running once when we first
>> need it.
>>
>> Any thoughts from anyone?  I don't think I can just do a lock drop here,
>> since other threads may jump in and mess with underlying state.  That
>> shouldn't affect the eDP state we fill out, but may affect the state the
>> caller depended on in the first place...
> 
> Also, has boot-time actually increased or did we simply push it somewhere
> we don't measure the delay anymore? After all right afterwards we'll do
> the fbcon setup, and that will synchronize everything again.

fbcon setup is pushed off to a work queue already.  This problem has
been around since before I posted the initial eDP work queue patch, and
is related to a few things: fetching the DPCD, EDID, and initializing
the panel power sequencer.  I think these days we actually do a PPS
cycle in eDP init too, which really increased the init time.

On one of my test systems here, module init time is about 1s.  With this
patch it drops to less than 300ms (most of that other time is spent in
power well functions; I still have to debug that).

> And on modern systems without fbcon I expect userspace to go around and do
> a probe, which again would force synchronization pretty quickly ...

Right, but the whole point is to get to userspace as soon as we can so
they'll at least have the option of poking us right away!

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

Re: Async eDP init

[Daniel Vetter]

On Thu, Mar 19, 2015 at 11:06:14AM -0700, Jesse Barnes wrote:
> On 03/19/2015 10:44 AM, Daniel Vetter wrote:
> > On Wed, Mar 18, 2015 at 11:41:48AM -0700, Jesse Barnes wrote:
> >> This updates my old patch for this, but w/o fixing the locking issue
> >> Ville mentioned.  In looking at it, it seems like the sync point should
> >> be at a higher level, maybe at the level of the atomic mode setting async
> >> serialization points?  Another possibility would be to make it a lazy
> >> init type function, sprinkled about but only running once when we first
> >> need it.
> >>
> >> Any thoughts from anyone?  I don't think I can just do a lock drop here,
> >> since other threads may jump in and mess with underlying state.  That
> >> shouldn't affect the eDP state we fill out, but may affect the state the
> >> caller depended on in the first place...
> > 
> > Also, has boot-time actually increased or did we simply push it somewhere
> > we don't measure the delay anymore? After all right afterwards we'll do
> > the fbcon setup, and that will synchronize everything again.
> 
> fbcon setup is pushed off to a work queue already.  This problem has
> been around since before I posted the initial eDP work queue patch, and
> is related to a few things: fetching the DPCD, EDID, and initializing
> the panel power sequencer.  I think these days we actually do a PPS
> cycle in eDP init too, which really increased the init time.

Hm I just read up on that patch again and noticed that the module load
code has an async_synchronize_full. Is there some magic I'm missing to not
make this synchronize with the fbdev stuff?

> On one of my test systems here, module init time is about 1s.  With this
> patch it drops to less than 300ms (most of that other time is spent in
> power well functions; I still have to debug that).
> 
> > And on modern systems without fbcon I expect userspace to go around and do
> > a probe, which again would force synchronization pretty quickly ...
> 
> Right, but the whole point is to get to userspace as soon as we can so
> they'll at least have the option of poking us right away!

Proper userspace forks one thread per module to load, so returning to
userspace fast isn't useful really. I'd really like to see some numbers
that this indeed makes boot-up faster before we add all the complexity
needed for it.
-Danel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Async eDP init

[Chris Wilson]

On Fri, Mar 20, 2015 at 11:16:28AM +0100, Daniel Vetter wrote:
> > Right, but the whole point is to get to userspace as soon as we can so
> > they'll at least have the option of poking us right away!
> 
> Proper userspace forks one thread per module to load, so returning to
> userspace fast isn't useful really. I'd really like to see some numbers
> that this indeed makes boot-up faster before we add all the complexity
> needed for it.

Modules? I presume the "instant on" systems will have pretty much the
base hardware (like i915) built in.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Async eDP init

[Jesse Barnes]

On 03/20/2015 03:16 AM, Daniel Vetter wrote:
> On Thu, Mar 19, 2015 at 11:06:14AM -0700, Jesse Barnes wrote:
>> On 03/19/2015 10:44 AM, Daniel Vetter wrote:
>>> On Wed, Mar 18, 2015 at 11:41:48AM -0700, Jesse Barnes wrote:
>>>> This updates my old patch for this, but w/o fixing the locking issue
>>>> Ville mentioned.  In looking at it, it seems like the sync point should
>>>> be at a higher level, maybe at the level of the atomic mode setting async
>>>> serialization points?  Another possibility would be to make it a lazy
>>>> init type function, sprinkled about but only running once when we first
>>>> need it.
>>>>
>>>> Any thoughts from anyone?  I don't think I can just do a lock drop here,
>>>> since other threads may jump in and mess with underlying state.  That
>>>> shouldn't affect the eDP state we fill out, but may affect the state the
>>>> caller depended on in the first place...
>>>
>>> Also, has boot-time actually increased or did we simply push it somewhere
>>> we don't measure the delay anymore? After all right afterwards we'll do
>>> the fbcon setup, and that will synchronize everything again.
>>
>> fbcon setup is pushed off to a work queue already.  This problem has
>> been around since before I posted the initial eDP work queue patch, and
>> is related to a few things: fetching the DPCD, EDID, and initializing
>> the panel power sequencer.  I think these days we actually do a PPS
>> cycle in eDP init too, which really increased the init time.
> 
> Hm I just read up on that patch again and noticed that the module load
> code has an async_synchronize_full. Is there some magic I'm missing to not
> make this synchronize with the fbdev stuff?

Maybe?  I'm not using async domains at all for this, so it doesn't sync
with anything until we get a call into the DP code.  So if fbcon comes
first and tries to set a mode, it'll end up in get_dpcd or some other
function which will flush the edp init work.

>> On one of my test systems here, module init time is about 1s.  With this
>> patch it drops to less than 300ms (most of that other time is spent in
>> power well functions; I still have to debug that).
>>
>>> And on modern systems without fbcon I expect userspace to go around and do
>>> a probe, which again would force synchronization pretty quickly ...
>>
>> Right, but the whole point is to get to userspace as soon as we can so
>> they'll at least have the option of poking us right away!
> 
> Proper userspace forks one thread per module to load, so returning to
> userspace fast isn't useful really. I'd really like to see some numbers
> that this indeed makes boot-up faster before we add all the complexity
> needed for it.

As Chris said, modules aren't always in use.  And I'd like to flip this
around too; you need to justify why spending over 1s in module init is
ok, modules or no.

Jesse

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