Re: [Intel-gfx] [PATCH v18 5/6] drm/i915/dp: Enable Upfront link training on DDI platforms

[Jani Nikula]

On Thu, 29 Sep 2016, Jani Nikula  wrote:
> On Wed, 21 Sep 2016, Manasi Navare  wrote:
>> To support USB type C alternate DP mode, the display driver needs to
>> know the number of lanes required by the DP panel as well as number
>> of lanes that can be supported by the type-C cable. Sometimes, the
>> type-C cable may limit the bandwidth even if Panel can support
>> more lanes. To address these scenarios we need to train the link before
>> modeset. This upfront link training caches the values of max link rate
>> and max lane count that get used later during modeset. Upfront link
>> training does not change any HW state, the link is disabled and PLL
>> values are reset to previous values after upfront link tarining so
>> that subsequent modeset is not aware of these changes.
>
> Some of the concerns and questions I've gathered about the upfront link
> training:
>
> * What if the userspace hasn't disabled the crtc by the time we get the
>   hotplug? Upfront link training just goes ahead and messes with that
>   state.
>
> * One of the potential benefits of upfront link training is that it
>   could make the hotplug faster by doing the link training before the
>   userspace even asks for a modeset. However, IIUC, the patch now does
>   the upfront link training, disables the link, and then does link
>   training again at modeset. Is that right? So it can actually make the
>   link training slower? (On the plus side, this avoids the problem of
>   leaving the link up and running if there isn't a userspace responding
>   to hotplug or userspace never asks for a modeset.)
>
> * Another benefit of upfront link training is that we can prune the
>   modes according to what the link can actually do, before
>   modeset. However, this still doesn't help the case of link degrading
>   during operation. (I am not sure how much we really care about this,
>   but it would seem that the approach described in my other mail might
>   solve it.)
>
> * Upfront link training is only enabled for DDI platforms, duplicating
>   parts of link training and apparently parts of modeset for DDI, and
>   diverging the link training code for DDI and non-DDI platforms. With
>   the current approach, this makes it impossible to run DP compliance
>   tests for non-DDI platforms.
>
> * How does upfront link training interact with atomic and fastboot? /me
>   clueless.
>
> * There just is a subjectively scary feeling to the change. The DP link
>   training code has been riddled with regressions in the past, and even
>   the smallest and innocent seeming changes have caused them. This is a
>   hard thing to justify, call it a gut feeling if you will, but history
>   has taught me not to dismiss those instincts with a shrug.
>
> All in all, I'd like to decouple current DP compliance efforts from
> upfront link training.

Another that I failed to mention: Eventually we'll want the DP
compliance stuff to be something that other drivers can have too. We'll
want to push more DP code to drm core DP helpers, and we'll want to
share the burden of keeping DP compliant. As the upfront link training
doesn't seem to be an (easy) option even for our non-DDI hardware, it
may be a difficult thing for other hardware as well. Maybe. Better stick
with something that's probably an easier sell for drm?

BR,
Jani.


>
> Ville, feel free to clarify or add to the list.
>
>
> BR,
> Jani.
>
>
>>
>> This patch is based on prior work done by
>> R,Durgadoss 
>>
>> Changes since v17:
>> * Rebased on the latest nightly
>> Changes since v16:
>> * Use HAS_DDI macro for enabling this feature (Rodrigo Vivi)
>> * Fix some unnecessary removals/changes due to rebase (Rodrigo Vivi)
>>
>> Changes since v15:
>> * Split this patch into two patches - one with functional
>> changes to enable upfront and other with moving the existing
>> functions around so that they can be used for upfront (Jani Nikula)
>> * Cleaned up the commit message
>>
>> Signed-off-by: Durgadoss R 
>> Signed-off-by: Jim Bride 
>> Signed-off-by: Manasi Navare 
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c  |  21 ++-
>>  drivers/gpu/drm/i915/intel_dp.c   | 190 
>> +-
>>  drivers/gpu/drm/i915/intel_dp_link_training.c |   1 -
>>  drivers/gpu/drm/i915/intel_drv.h  |  14 +-
>>  4 files changed, 218 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
>> b/drivers/gpu/drm/i915/intel_ddi.c
>> index 093038c..8e52507 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1676,7 +1676,8 @@ static void intel_ddi_pre_enable_dp(struct 
>> intel_encoder *encoder,
>>  pll->config.crtc_mask = 0;
>>  
>>  /* If Link Training fails, send a uevent to generate a hotplug */
>> -if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst))
>> +if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst,
>> +  false))
>>  drm_kms_helper_h

Re: [Intel-gfx] [PATCH v18 5/6] drm/i915/dp: Enable Upfront link training on DDI platforms

[Jani Nikula]

On Wed, 21 Sep 2016, Manasi Navare  wrote:
> To support USB type C alternate DP mode, the display driver needs to
> know the number of lanes required by the DP panel as well as number
> of lanes that can be supported by the type-C cable. Sometimes, the
> type-C cable may limit the bandwidth even if Panel can support
> more lanes. To address these scenarios we need to train the link before
> modeset. This upfront link training caches the values of max link rate
> and max lane count that get used later during modeset. Upfront link
> training does not change any HW state, the link is disabled and PLL
> values are reset to previous values after upfront link tarining so
> that subsequent modeset is not aware of these changes.

Some of the concerns and questions I've gathered about the upfront link
training:

* What if the userspace hasn't disabled the crtc by the time we get the
  hotplug? Upfront link training just goes ahead and messes with that
  state.

* One of the potential benefits of upfront link training is that it
  could make the hotplug faster by doing the link training before the
  userspace even asks for a modeset. However, IIUC, the patch now does
  the upfront link training, disables the link, and then does link
  training again at modeset. Is that right? So it can actually make the
  link training slower? (On the plus side, this avoids the problem of
  leaving the link up and running if there isn't a userspace responding
  to hotplug or userspace never asks for a modeset.)

* Another benefit of upfront link training is that we can prune the
  modes according to what the link can actually do, before
  modeset. However, this still doesn't help the case of link degrading
  during operation. (I am not sure how much we really care about this,
  but it would seem that the approach described in my other mail might
  solve it.)

* Upfront link training is only enabled for DDI platforms, duplicating
  parts of link training and apparently parts of modeset for DDI, and
  diverging the link training code for DDI and non-DDI platforms. With
  the current approach, this makes it impossible to run DP compliance
  tests for non-DDI platforms.

* How does upfront link training interact with atomic and fastboot? /me
  clueless.

* There just is a subjectively scary feeling to the change. The DP link
  training code has been riddled with regressions in the past, and even
  the smallest and innocent seeming changes have caused them. This is a
  hard thing to justify, call it a gut feeling if you will, but history
  has taught me not to dismiss those instincts with a shrug.

All in all, I'd like to decouple current DP compliance efforts from
upfront link training.

Ville, feel free to clarify or add to the list.


BR,
Jani.


>
> This patch is based on prior work done by
> R,Durgadoss 
>
> Changes since v17:
> * Rebased on the latest nightly
> Changes since v16:
> * Use HAS_DDI macro for enabling this feature (Rodrigo Vivi)
> * Fix some unnecessary removals/changes due to rebase (Rodrigo Vivi)
>
> Changes since v15:
> * Split this patch into two patches - one with functional
> changes to enable upfront and other with moving the existing
> functions around so that they can be used for upfront (Jani Nikula)
> * Cleaned up the commit message
>
> Signed-off-by: Durgadoss R 
> Signed-off-by: Jim Bride 
> Signed-off-by: Manasi Navare 
> ---
>  drivers/gpu/drm/i915/intel_ddi.c  |  21 ++-
>  drivers/gpu/drm/i915/intel_dp.c   | 190 
> +-
>  drivers/gpu/drm/i915/intel_dp_link_training.c |   1 -
>  drivers/gpu/drm/i915/intel_drv.h  |  14 +-
>  4 files changed, 218 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 093038c..8e52507 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1676,7 +1676,8 @@ static void intel_ddi_pre_enable_dp(struct 
> intel_encoder *encoder,
>   pll->config.crtc_mask = 0;
>  
>   /* If Link Training fails, send a uevent to generate a hotplug */
> - if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst))
> + if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst,
> +   false))
>   drm_kms_helper_hotplug_event(encoder->base.dev);
>   pll->config = tmp_pll_config;
>  }
> @@ -2464,7 +2465,7 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int 
> clock)
>  
>  bool
>  intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate,
> -  uint8_t max_lane_count, bool link_mst)
> +  uint8_t max_lane_count, bool link_mst, bool is_upfront)
>  {
>   struct intel_connector *connector = intel_dp->attached_connector;
>   struct intel_encoder *encoder = connector->encoder;
> @@ -2513,6 +2514,7 @@ intel_ddi_link_train(struct intel_dp *intel_dp, int 
> max_link_rate,
>   pll->funcs.disable

Re: [Intel-gfx] [PATCH v18 5/6] drm/i915/dp: Enable Upfront link training on DDI platforms

[Jani Nikula]

On Wed, 21 Sep 2016, Manasi Navare  wrote:
> To support USB type C alternate DP mode, the display driver needs to
> know the number of lanes required by the DP panel as well as number
> of lanes that can be supported by the type-C cable. Sometimes, the
> type-C cable may limit the bandwidth even if Panel can support
> more lanes. To address these scenarios we need to train the link before
> modeset. This upfront link training caches the values of max link rate
> and max lane count that get used later during modeset. Upfront link
> training does not change any HW state, the link is disabled and PLL
> values are reset to previous values after upfront link tarining so
> that subsequent modeset is not aware of these changes.

I think we should call timeout on this patch, and focus on the DP
compliance parts first. Frankly, I think this patch is really scary. If
I got a bisect result for a regression on this, I would have absolutely
no clue what exactly caused it.

Please correct me if you think I'm wrong, but I don't think upfront link
training is strictly required for DP compliance. (Conversely, if you
think this is required for DP compliance, the rationale is absolutely
required in the commit message in more than just a few words.)

BR,
Jani.


>
> This patch is based on prior work done by
> R,Durgadoss 
>
> Changes since v17:
> * Rebased on the latest nightly
> Changes since v16:
> * Use HAS_DDI macro for enabling this feature (Rodrigo Vivi)
> * Fix some unnecessary removals/changes due to rebase (Rodrigo Vivi)
>
> Changes since v15:
> * Split this patch into two patches - one with functional
> changes to enable upfront and other with moving the existing
> functions around so that they can be used for upfront (Jani Nikula)
> * Cleaned up the commit message
>
> Signed-off-by: Durgadoss R 
> Signed-off-by: Jim Bride 
> Signed-off-by: Manasi Navare 
> ---
>  drivers/gpu/drm/i915/intel_ddi.c  |  21 ++-
>  drivers/gpu/drm/i915/intel_dp.c   | 190 
> +-
>  drivers/gpu/drm/i915/intel_dp_link_training.c |   1 -
>  drivers/gpu/drm/i915/intel_drv.h  |  14 +-
>  4 files changed, 218 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 093038c..8e52507 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1676,7 +1676,8 @@ static void intel_ddi_pre_enable_dp(struct 
> intel_encoder *encoder,
>   pll->config.crtc_mask = 0;
>  
>   /* If Link Training fails, send a uevent to generate a hotplug */
> - if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst))
> + if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst,
> +   false))
>   drm_kms_helper_hotplug_event(encoder->base.dev);
>   pll->config = tmp_pll_config;
>  }
> @@ -2464,7 +2465,7 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int 
> clock)
>  
>  bool
>  intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate,
> -  uint8_t max_lane_count, bool link_mst)
> +  uint8_t max_lane_count, bool link_mst, bool is_upfront)
>  {
>   struct intel_connector *connector = intel_dp->attached_connector;
>   struct intel_encoder *encoder = connector->encoder;
> @@ -2513,6 +2514,7 @@ intel_ddi_link_train(struct intel_dp *intel_dp, int 
> max_link_rate,
>   pll->funcs.disable(dev_priv, pll);
>   pll->config = tmp_pll_config;
>   }
> +
>   if (ret) {
>   DRM_DEBUG_KMS("Link Training successful at link rate: 
> %d lane: %d\n",
> link_rate, lane_count);
> @@ -2522,6 +2524,21 @@ intel_ddi_link_train(struct intel_dp *intel_dp, int 
> max_link_rate,
>  
>   intel_dp_stop_link_train(intel_dp);
>  
> + if (is_upfront) {
> + DRM_DEBUG_KMS("Upfront link train %s: link_clock:%d lanes:%d\n",
> +   ret ? "Passed" : "Failed",
> +   link_rate, lane_count);
> + /* Disable port followed by PLL for next retry/clean up */
> + intel_ddi_post_disable(encoder, NULL, NULL);
> + pll->funcs.disable(dev_priv, pll);
> + pll->config = tmp_pll_config;
> + if (ret) {
> + /* Save the upfront values */
> + intel_dp->max_lanes_upfront = lane_count;
> + intel_dp->max_link_rate_upfront = link_rate;
> + }
> + }
> +
>   if (!lane_count)
>   DRM_ERROR("Link Training Failed\n");
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8d9a8ab..a058d5d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -153,12 +153,21 @@ intel_dp_max_link_bw(struct intel_dp  *intel_dp)
>  static u8 intel_dp_max_lane_c

[Intel-gfx] [PATCH v18 5/6] drm/i915/dp: Enable Upfront link training on DDI platforms

[Manasi Navare]

To support USB type C alternate DP mode, the display driver needs to
know the number of lanes required by the DP panel as well as number
of lanes that can be supported by the type-C cable. Sometimes, the
type-C cable may limit the bandwidth even if Panel can support
more lanes. To address these scenarios we need to train the link before
modeset. This upfront link training caches the values of max link rate
and max lane count that get used later during modeset. Upfront link
training does not change any HW state, the link is disabled and PLL
values are reset to previous values after upfront link tarining so
that subsequent modeset is not aware of these changes.

This patch is based on prior work done by
R,Durgadoss 

Changes since v17:
* Rebased on the latest nightly
Changes since v16:
* Use HAS_DDI macro for enabling this feature (Rodrigo Vivi)
* Fix some unnecessary removals/changes due to rebase (Rodrigo Vivi)

Changes since v15:
* Split this patch into two patches - one with functional
changes to enable upfront and other with moving the existing
functions around so that they can be used for upfront (Jani Nikula)
* Cleaned up the commit message

Signed-off-by: Durgadoss R 
Signed-off-by: Jim Bride 
Signed-off-by: Manasi Navare 
---
 drivers/gpu/drm/i915/intel_ddi.c  |  21 ++-
 drivers/gpu/drm/i915/intel_dp.c   | 190 +-
 drivers/gpu/drm/i915/intel_dp_link_training.c |   1 -
 drivers/gpu/drm/i915/intel_drv.h  |  14 +-
 4 files changed, 218 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 093038c..8e52507 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1676,7 +1676,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder 
*encoder,
pll->config.crtc_mask = 0;
 
/* If Link Training fails, send a uevent to generate a hotplug */
-   if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst))
+   if (!intel_ddi_link_train(intel_dp, link_rate, lane_count, link_mst,
+ false))
drm_kms_helper_hotplug_event(encoder->base.dev);
pll->config = tmp_pll_config;
 }
@@ -2464,7 +2465,7 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int 
clock)
 
 bool
 intel_ddi_link_train(struct intel_dp *intel_dp, int max_link_rate,
-uint8_t max_lane_count, bool link_mst)
+uint8_t max_lane_count, bool link_mst, bool is_upfront)
 {
struct intel_connector *connector = intel_dp->attached_connector;
struct intel_encoder *encoder = connector->encoder;
@@ -2513,6 +2514,7 @@ intel_ddi_link_train(struct intel_dp *intel_dp, int 
max_link_rate,
pll->funcs.disable(dev_priv, pll);
pll->config = tmp_pll_config;
}
+
if (ret) {
DRM_DEBUG_KMS("Link Training successful at link rate: 
%d lane: %d\n",
  link_rate, lane_count);
@@ -2522,6 +2524,21 @@ intel_ddi_link_train(struct intel_dp *intel_dp, int 
max_link_rate,
 
intel_dp_stop_link_train(intel_dp);
 
+   if (is_upfront) {
+   DRM_DEBUG_KMS("Upfront link train %s: link_clock:%d lanes:%d\n",
+ ret ? "Passed" : "Failed",
+ link_rate, lane_count);
+   /* Disable port followed by PLL for next retry/clean up */
+   intel_ddi_post_disable(encoder, NULL, NULL);
+   pll->funcs.disable(dev_priv, pll);
+   pll->config = tmp_pll_config;
+   if (ret) {
+   /* Save the upfront values */
+   intel_dp->max_lanes_upfront = lane_count;
+   intel_dp->max_link_rate_upfront = link_rate;
+   }
+   }
+
if (!lane_count)
DRM_ERROR("Link Training Failed\n");
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8d9a8ab..a058d5d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -153,12 +153,21 @@ intel_dp_max_link_bw(struct intel_dp  *intel_dp)
 static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)
 {
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-   u8 source_max, sink_max;
+   u8 temp, source_max, sink_max;
 
source_max = intel_dig_port->max_lanes;
sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
 
-   return min(source_max, sink_max);
+   temp = min(source_max, sink_max);
+
+   /*
+* Limit max lanes w.r.t to the max value found
+* using Upfront link training also.
+*/
+   if (intel_dp->max_lanes_upfront)
+   return min(temp, intel_dp->max_lanes_upfront);
+   else
+   return temp;
 }
 
 /*
@@ -190,6 +199,42 @@ intel_dp_max_data_rate(int