| Message ID | 20180912225719.17574-1-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/4] drm/i915/dp: Link train Fallback on eDP only if fallback link BW can fit panel's native mode | expand |
On Wed, Sep 12, 2018 at 3:55 PM Manasi Navare <manasi.d.navare@intel.com> wrote: > > This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp: > Do not do link training fallback or prune modes on EDP") that causes > a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350) > where first link training fails and a retraining is required by falling > back to lower link rate/lane count. > In case of some panels they advertise higher link rate/lane count > than whats required for supporting the panel's native mode. > But we always link train at highest link rate/lane count for eDP > and if that fails we can still fallback to lower link rate/lane count > as long as the fallback link BW still fits the native mode to avoid > pruning the panel's native mode yet retraining at fallback values > to recover from a blank screen. I suspect something went bad on my laptop or the cause is something else. I can't reproduce this problem anymore, even if I go back to old kernels. And, in contrary to what I had previously, now I have: # dd if=/dev/drm_dp_aux0 bs=1 | hexdump -C ... 00000070 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| ... Lucas De Marchi > > v2: > * Send uevent if link failure on eDP unconditionally > > Cc: Clinton Taylor <clinton.a.taylor@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338 > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > Tested-by: Alexander Wilson <alexander.wilson@ncf.edu> > --- > drivers/gpu/drm/i915/intel_dp.c | 29 +++++++++++++++++++ > drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++----------- > 2 files changed, 38 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 436c22de33b6..e4de5257cd87 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate, > return true; > } > > +static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp, > + int link_rate, > + uint8_t lane_count) > +{ > + struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode; > + int mode_rate, max_rate; > + > + mode_rate = intel_dp_link_required(fixed_mode->clock, 18); > + max_rate = intel_dp_max_data_rate(link_rate, lane_count); > + if (mode_rate > max_rate) > + return false; > + > + return true; > +} > + > int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > int link_rate, uint8_t lane_count) > { > @@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > intel_dp->num_common_rates, > link_rate); > if (index > 0) { > + if (intel_dp_is_edp(intel_dp) && > + !intel_dp_can_link_train_fallback_for_edp(intel_dp, > + intel_dp->common_rates[index - 1], > + lane_count)) { > + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); > + return 0; > + } > intel_dp->max_link_rate = intel_dp->common_rates[index - 1]; > intel_dp->max_link_lane_count = lane_count; > } else if (lane_count > 1) { > + if (intel_dp_is_edp(intel_dp) && > + !intel_dp_can_link_train_fallback_for_edp(intel_dp, > + intel_dp_max_common_rate(intel_dp), > + lane_count >> 1)) { > + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); > + return 0; > + } > intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp); > intel_dp->max_link_lane_count = lane_count >> 1; > } else { > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > index a9f40985a621..30be0e39bd5f 100644 > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > @@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > return; > > failure_handling: > - /* Dont fallback and prune modes if its eDP */ > - if (!intel_dp_is_edp(intel_dp)) { > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > - intel_connector->base.base.id, > - intel_connector->base.name, > - intel_dp->link_rate, intel_dp->lane_count); > - if (!intel_dp_get_link_train_fallback_values(intel_dp, > - intel_dp->link_rate, > - intel_dp->lane_count)) > - /* Schedule a Hotplug Uevent to userspace to start modeset */ > - schedule_work(&intel_connector->modeset_retry_work); > - } else { > - DRM_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > - intel_connector->base.base.id, > - intel_connector->base.name, > - intel_dp->link_rate, intel_dp->lane_count); > - } > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > + intel_connector->base.base.id, > + intel_connector->base.name, > + intel_dp->link_rate, intel_dp->lane_count); > + if (!intel_dp_get_link_train_fallback_values(intel_dp, > + intel_dp->link_rate, > + intel_dp->lane_count)) > + /* Schedule a Hotplug Uevent to userspace to start modeset */ > + schedule_work(&intel_connector->modeset_retry_work); > return; > } > -- > 2.18.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Sep 12, 2018 at 03:57:16PM -0700, Manasi Navare wrote: > This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp: > Do not do link training fallback or prune modes on EDP") that causes > a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350) > where first link training fails and a retraining is required by falling > back to lower link rate/lane count. > In case of some panels they advertise higher link rate/lane count > than whats required for supporting the panel's native mode. > But we always link train at highest link rate/lane count for eDP > and if that fails we can still fallback to lower link rate/lane count > as long as the fallback link BW still fits the native mode to avoid > pruning the panel's native mode yet retraining at fallback values > to recover from a blank screen. > > v2: > * Send uevent if link failure on eDP unconditionally > > Cc: Clinton Taylor <clinton.a.taylor@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338 > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > Tested-by: Alexander Wilson <alexander.wilson@ncf.edu> > --- > drivers/gpu/drm/i915/intel_dp.c | 29 +++++++++++++++++++ > drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++----------- > 2 files changed, 38 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 436c22de33b6..e4de5257cd87 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate, > return true; > } > > +static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp, > + int link_rate, > + uint8_t lane_count) > +{ > + struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode; const > + int mode_rate, max_rate; > + > + mode_rate = intel_dp_link_required(fixed_mode->clock, 18); > + max_rate = intel_dp_max_data_rate(link_rate, lane_count); > + if (mode_rate > max_rate) > + return false; I wonder if we should extract this into a helper and share with mode_valid(). Then again, it's just a few lines so maybe not worth it. > + > + return true; > +} > + > int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > int link_rate, uint8_t lane_count) > { > @@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > intel_dp->num_common_rates, > link_rate); > if (index > 0) { > + if (intel_dp_is_edp(intel_dp) && > + !intel_dp_can_link_train_fallback_for_edp(intel_dp, > + intel_dp->common_rates[index - 1], > + lane_count)) { > + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); > + return 0; > + } > intel_dp->max_link_rate = intel_dp->common_rates[index - 1]; > intel_dp->max_link_lane_count = lane_count; > } else if (lane_count > 1) { > + if (intel_dp_is_edp(intel_dp) && > + !intel_dp_can_link_train_fallback_for_edp(intel_dp, > + intel_dp_max_common_rate(intel_dp), > + lane_count >> 1)) { > + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); > + return 0; > + } > intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp); > intel_dp->max_link_lane_count = lane_count >> 1; This whole thing is getting a bit messy. I think it would worthwile to rewrite this as something like: intel_dp_update_link_train_fallback_values(intel_dp) { int link_rate = intel_dp->link_rate; int lane_count = intel_dp->lane_count; if (intel_dp_get_link_train_fallback_values(intel_dp, &link_rate, &lane_count)) return -1; if (is_edp() && !edp_can_link_train_thing(link_rate, lane_count)) { DRM_DEBUG_KMS("..."); return 0; } intel_dp->max_link_rate = link_rate; intel_dp->max_link_lane_count = lane_count; return 0; } But that can be done later. With the missing const added this patch is Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > } else { > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > index a9f40985a621..30be0e39bd5f 100644 > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > @@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > return; > > failure_handling: > - /* Dont fallback and prune modes if its eDP */ > - if (!intel_dp_is_edp(intel_dp)) { > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > - intel_connector->base.base.id, > - intel_connector->base.name, > - intel_dp->link_rate, intel_dp->lane_count); > - if (!intel_dp_get_link_train_fallback_values(intel_dp, > - intel_dp->link_rate, > - intel_dp->lane_count)) > - /* Schedule a Hotplug Uevent to userspace to start modeset */ > - schedule_work(&intel_connector->modeset_retry_work); > - } else { > - DRM_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > - intel_connector->base.base.id, > - intel_connector->base.name, > - intel_dp->link_rate, intel_dp->lane_count); > - } > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > + intel_connector->base.base.id, > + intel_connector->base.name, > + intel_dp->link_rate, intel_dp->lane_count); > + if (!intel_dp_get_link_train_fallback_values(intel_dp, > + intel_dp->link_rate, > + intel_dp->lane_count)) > + /* Schedule a Hotplug Uevent to userspace to start modeset */ > + schedule_work(&intel_connector->modeset_retry_work); > return; > } > -- > 2.18.0
On Mon, Oct 01, 2018 at 07:03:52PM +0300, Ville Syrjälä wrote: > On Wed, Sep 12, 2018 at 03:57:16PM -0700, Manasi Navare wrote: > > This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp: > > Do not do link training fallback or prune modes on EDP") that causes > > a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350) > > where first link training fails and a retraining is required by falling > > back to lower link rate/lane count. > > In case of some panels they advertise higher link rate/lane count > > than whats required for supporting the panel's native mode. > > But we always link train at highest link rate/lane count for eDP > > and if that fails we can still fallback to lower link rate/lane count > > as long as the fallback link BW still fits the native mode to avoid > > pruning the panel's native mode yet retraining at fallback values > > to recover from a blank screen. > > > > v2: > > * Send uevent if link failure on eDP unconditionally > > > > Cc: Clinton Taylor <clinton.a.taylor@intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338 > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > Tested-by: Alexander Wilson <alexander.wilson@ncf.edu> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 29 +++++++++++++++++++ > > drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++----------- > > 2 files changed, 38 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 436c22de33b6..e4de5257cd87 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate, > > return true; > > } > > > > +static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp, > > + int link_rate, > > + uint8_t lane_count) > > +{ > > + struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode; > > const > Will add const to the fixed_mode. > > + int mode_rate, max_rate; > > + > > + mode_rate = intel_dp_link_required(fixed_mode->clock, 18); > > + max_rate = intel_dp_max_data_rate(link_rate, lane_count); > > + if (mode_rate > max_rate) > > + return false; > > I wonder if we should extract this into a helper and share with > mode_valid(). Then again, it's just a few lines so maybe not worth it. > > > + > > + return true; > > +} > > + > > int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > int link_rate, uint8_t lane_count) > > { > > @@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > intel_dp->num_common_rates, > > link_rate); > > if (index > 0) { > > + if (intel_dp_is_edp(intel_dp) && > > + !intel_dp_can_link_train_fallback_for_edp(intel_dp, > > + intel_dp->common_rates[index - 1], > > + lane_count)) { > > + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); > > + return 0; > > + } > > intel_dp->max_link_rate = intel_dp->common_rates[index - 1]; > > intel_dp->max_link_lane_count = lane_count; > > } else if (lane_count > 1) { > > + if (intel_dp_is_edp(intel_dp) && > > + !intel_dp_can_link_train_fallback_for_edp(intel_dp, > > + intel_dp_max_common_rate(intel_dp), > > + lane_count >> 1)) { > > + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); > > + return 0; > > + } > > intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp); > > intel_dp->max_link_lane_count = lane_count >> 1; > > This whole thing is getting a bit messy. I think it would worthwile to > rewrite this as something like: > > intel_dp_update_link_train_fallback_values(intel_dp) > { > int link_rate = intel_dp->link_rate; > int lane_count = intel_dp->lane_count; > > if (intel_dp_get_link_train_fallback_values(intel_dp, &link_rate, &lane_count)) > return -1; > > if (is_edp() && !edp_can_link_train_thing(link_rate, lane_count)) { > DRM_DEBUG_KMS("..."); > return 0; > } > > intel_dp->max_link_rate = link_rate; > intel_dp->max_link_lane_count = lane_count; > > return 0; > } > > But that can be done later. With the missing const added this patch is > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Yes this can be refactored/reorganized as a follow up patch, also when downclock mode checks get in. So until then thsi fix is good to be merged right? Manasi > > } else { > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > > index a9f40985a621..30be0e39bd5f 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > @@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > > return; > > > > failure_handling: > > - /* Dont fallback and prune modes if its eDP */ > > - if (!intel_dp_is_edp(intel_dp)) { > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > > - intel_connector->base.base.id, > > - intel_connector->base.name, > > - intel_dp->link_rate, intel_dp->lane_count); > > - if (!intel_dp_get_link_train_fallback_values(intel_dp, > > - intel_dp->link_rate, > > - intel_dp->lane_count)) > > - /* Schedule a Hotplug Uevent to userspace to start modeset */ > > - schedule_work(&intel_connector->modeset_retry_work); > > - } else { > > - DRM_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > > - intel_connector->base.base.id, > > - intel_connector->base.name, > > - intel_dp->link_rate, intel_dp->lane_count); > > - } > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > > + intel_connector->base.base.id, > > + intel_connector->base.name, > > + intel_dp->link_rate, intel_dp->lane_count); > > + if (!intel_dp_get_link_train_fallback_values(intel_dp, > > + intel_dp->link_rate, > > + intel_dp->lane_count)) > > + /* Schedule a Hotplug Uevent to userspace to start modeset */ > > + schedule_work(&intel_connector->modeset_retry_work); > > return; > > } > > -- > > 2.18.0 > > -- > Ville Syrjälä > Intel
On Mon, Oct 08, 2018 at 12:23:32PM -0700, Manasi Navare wrote: > On Mon, Oct 01, 2018 at 07:03:52PM +0300, Ville Syrjälä wrote: > > On Wed, Sep 12, 2018 at 03:57:16PM -0700, Manasi Navare wrote: > > > This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp: > > > Do not do link training fallback or prune modes on EDP") that causes > > > a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350) > > > where first link training fails and a retraining is required by falling > > > back to lower link rate/lane count. > > > In case of some panels they advertise higher link rate/lane count > > > than whats required for supporting the panel's native mode. > > > But we always link train at highest link rate/lane count for eDP > > > and if that fails we can still fallback to lower link rate/lane count > > > as long as the fallback link BW still fits the native mode to avoid > > > pruning the panel's native mode yet retraining at fallback values > > > to recover from a blank screen. > > > > > > v2: > > > * Send uevent if link failure on eDP unconditionally > > > > > > Cc: Clinton Taylor <clinton.a.taylor@intel.com> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489 > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338 > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > Tested-by: Alexander Wilson <alexander.wilson@ncf.edu> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 29 +++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++----------- > > > 2 files changed, 38 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index 436c22de33b6..e4de5257cd87 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate, > > > return true; > > > } > > > > > > +static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp, > > > + int link_rate, > > > + uint8_t lane_count) > > > +{ > > > + struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode; > > > > const > > > > Will add const to the fixed_mode. > > > > + int mode_rate, max_rate; > > > + > > > + mode_rate = intel_dp_link_required(fixed_mode->clock, 18); > > > + max_rate = intel_dp_max_data_rate(link_rate, lane_count); > > > + if (mode_rate > max_rate) > > > + return false; > > > > I wonder if we should extract this into a helper and share with > > mode_valid(). Then again, it's just a few lines so maybe not worth it. > > > > > + > > > + return true; > > > +} > > > + > > > int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > > int link_rate, uint8_t lane_count) > > > { > > > @@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > > intel_dp->num_common_rates, > > > link_rate); > > > if (index > 0) { > > > + if (intel_dp_is_edp(intel_dp) && > > > + !intel_dp_can_link_train_fallback_for_edp(intel_dp, > > > + intel_dp->common_rates[index - 1], > > > + lane_count)) { > > > + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); > > > + return 0; > > > + } > > > intel_dp->max_link_rate = intel_dp->common_rates[index - 1]; > > > intel_dp->max_link_lane_count = lane_count; > > > } else if (lane_count > 1) { > > > + if (intel_dp_is_edp(intel_dp) && > > > + !intel_dp_can_link_train_fallback_for_edp(intel_dp, > > > + intel_dp_max_common_rate(intel_dp), > > > + lane_count >> 1)) { > > > + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); > > > + return 0; > > > + } > > > intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp); > > > intel_dp->max_link_lane_count = lane_count >> 1; > > > > This whole thing is getting a bit messy. I think it would worthwile to > > rewrite this as something like: > > > > intel_dp_update_link_train_fallback_values(intel_dp) > > { > > int link_rate = intel_dp->link_rate; > > int lane_count = intel_dp->lane_count; > > > > if (intel_dp_get_link_train_fallback_values(intel_dp, &link_rate, &lane_count)) > > return -1; > > > > if (is_edp() && !edp_can_link_train_thing(link_rate, lane_count)) { > > DRM_DEBUG_KMS("..."); > > return 0; > > } > > > > intel_dp->max_link_rate = link_rate; > > intel_dp->max_link_lane_count = lane_count; > > > > return 0; > > } > > > > But that can be done later. With the missing const added this patch is > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Yes this can be refactored/reorganized as a follow up patch, also when downclock mode > checks get in. > So until then thsi fix is good to be merged right? Yes. > > Manasi > > > > } else { > > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > > > index a9f40985a621..30be0e39bd5f 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > > @@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > > > return; > > > > > > failure_handling: > > > - /* Dont fallback and prune modes if its eDP */ > > > - if (!intel_dp_is_edp(intel_dp)) { > > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > > > - intel_connector->base.base.id, > > > - intel_connector->base.name, > > > - intel_dp->link_rate, intel_dp->lane_count); > > > - if (!intel_dp_get_link_train_fallback_values(intel_dp, > > > - intel_dp->link_rate, > > > - intel_dp->lane_count)) > > > - /* Schedule a Hotplug Uevent to userspace to start modeset */ > > > - schedule_work(&intel_connector->modeset_retry_work); > > > - } else { > > > - DRM_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > > > - intel_connector->base.base.id, > > > - intel_connector->base.name, > > > - intel_dp->link_rate, intel_dp->lane_count); > > > - } > > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > > > + intel_connector->base.base.id, > > > + intel_connector->base.name, > > > + intel_dp->link_rate, intel_dp->lane_count); > > > + if (!intel_dp_get_link_train_fallback_values(intel_dp, > > > + intel_dp->link_rate, > > > + intel_dp->lane_count)) > > > + /* Schedule a Hotplug Uevent to userspace to start modeset */ > > > + schedule_work(&intel_connector->modeset_retry_work); > > > return; > > > } > > > -- > > > 2.18.0 > > > > -- > > Ville Syrjälä > > Intel
On Wed, Sep 12, 2018 at 05:03:15PM -0700, Lucas De Marchi wrote: > On Wed, Sep 12, 2018 at 3:55 PM Manasi Navare <manasi.d.navare@intel.com> wrote: > > > > This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp: > > Do not do link training fallback or prune modes on EDP") that causes > > a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350) > > where first link training fails and a retraining is required by falling > > back to lower link rate/lane count. > > In case of some panels they advertise higher link rate/lane count > > than whats required for supporting the panel's native mode. > > But we always link train at highest link rate/lane count for eDP > > and if that fails we can still fallback to lower link rate/lane count > > as long as the fallback link BW still fits the native mode to avoid > > pruning the panel's native mode yet retraining at fallback values > > to recover from a blank screen. > > I suspect something went bad on my laptop or the cause is something else. > I can't reproduce this problem anymore, even if I go back to old kernels. > And, in contrary to what I had previously, now I have: > > # dd if=/dev/drm_dp_aux0 bs=1 | hexdump -C > ... > 00000070 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| > ... > Thanks for testing this scenario again. I think this is one of those panels where sometimes it works without a retrain and sometimes requires retraining which was the cause of the black screen on your laptop earlier. But regardless there are other bugs seen where where we need retraining on eDP with reduced fallback parameters or the same parameters. And this patch still fixes those bugs. Manasi > Lucas De Marchi > > > > > v2: > > * Send uevent if link failure on eDP unconditionally > > > > Cc: Clinton Taylor <clinton.a.taylor@intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338 > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > Tested-by: Alexander Wilson <alexander.wilson@ncf.edu> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 29 +++++++++++++++++++ > > drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++----------- > > 2 files changed, 38 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 436c22de33b6..e4de5257cd87 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate, > > return true; > > } > > > > +static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp, > > + int link_rate, > > + uint8_t lane_count) > > +{ > > + struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode; > > + int mode_rate, max_rate; > > + > > + mode_rate = intel_dp_link_required(fixed_mode->clock, 18); > > + max_rate = intel_dp_max_data_rate(link_rate, lane_count); > > + if (mode_rate > max_rate) > > + return false; > > + > > + return true; > > +} > > + > > int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > int link_rate, uint8_t lane_count) > > { > > @@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > intel_dp->num_common_rates, > > link_rate); > > if (index > 0) { > > + if (intel_dp_is_edp(intel_dp) && > > + !intel_dp_can_link_train_fallback_for_edp(intel_dp, > > + intel_dp->common_rates[index - 1], > > + lane_count)) { > > + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); > > + return 0; > > + } > > intel_dp->max_link_rate = intel_dp->common_rates[index - 1]; > > intel_dp->max_link_lane_count = lane_count; > > } else if (lane_count > 1) { > > + if (intel_dp_is_edp(intel_dp) && > > + !intel_dp_can_link_train_fallback_for_edp(intel_dp, > > + intel_dp_max_common_rate(intel_dp), > > + lane_count >> 1)) { > > + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); > > + return 0; > > + } > > intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp); > > intel_dp->max_link_lane_count = lane_count >> 1; > > } else { > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > > index a9f40985a621..30be0e39bd5f 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > @@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > > return; > > > > failure_handling: > > - /* Dont fallback and prune modes if its eDP */ > > - if (!intel_dp_is_edp(intel_dp)) { > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > > - intel_connector->base.base.id, > > - intel_connector->base.name, > > - intel_dp->link_rate, intel_dp->lane_count); > > - if (!intel_dp_get_link_train_fallback_values(intel_dp, > > - intel_dp->link_rate, > > - intel_dp->lane_count)) > > - /* Schedule a Hotplug Uevent to userspace to start modeset */ > > - schedule_work(&intel_connector->modeset_retry_work); > > - } else { > > - DRM_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > > - intel_connector->base.base.id, > > - intel_connector->base.name, > > - intel_dp->link_rate, intel_dp->lane_count); > > - } > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", > > + intel_connector->base.base.id, > > + intel_connector->base.name, > > + intel_dp->link_rate, intel_dp->lane_count); > > + if (!intel_dp_get_link_train_fallback_values(intel_dp, > > + intel_dp->link_rate, > > + intel_dp->lane_count)) > > + /* Schedule a Hotplug Uevent to userspace to start modeset */ > > + schedule_work(&intel_connector->modeset_retry_work); > > return; > > } > > -- > > 2.18.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Lucas De Marchi
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 436c22de33b6..e4de5257cd87 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -557,6 +557,21 @@ static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int link_rate, return true; } +static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp, + int link_rate, + uint8_t lane_count) +{ + struct drm_display_mode *fixed_mode = intel_dp->attached_connector->panel.fixed_mode; + int mode_rate, max_rate; + + mode_rate = intel_dp_link_required(fixed_mode->clock, 18); + max_rate = intel_dp_max_data_rate(link_rate, lane_count); + if (mode_rate > max_rate) + return false; + + return true; +} + int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, int link_rate, uint8_t lane_count) { @@ -566,9 +581,23 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, intel_dp->num_common_rates, link_rate); if (index > 0) { + if (intel_dp_is_edp(intel_dp) && + !intel_dp_can_link_train_fallback_for_edp(intel_dp, + intel_dp->common_rates[index - 1], + lane_count)) { + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); + return 0; + } intel_dp->max_link_rate = intel_dp->common_rates[index - 1]; intel_dp->max_link_lane_count = lane_count; } else if (lane_count > 1) { + if (intel_dp_is_edp(intel_dp) && + !intel_dp_can_link_train_fallback_for_edp(intel_dp, + intel_dp_max_common_rate(intel_dp), + lane_count >> 1)) { + DRM_DEBUG_KMS("Retrying Link training for eDP with same parameters\n"); + return 0; + } intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp); intel_dp->max_link_lane_count = lane_count >> 1; } else { diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index a9f40985a621..30be0e39bd5f 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -367,22 +367,14 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) return; failure_handling: - /* Dont fallback and prune modes if its eDP */ - if (!intel_dp_is_edp(intel_dp)) { - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", - intel_connector->base.base.id, - intel_connector->base.name, - intel_dp->link_rate, intel_dp->lane_count); - if (!intel_dp_get_link_train_fallback_values(intel_dp, - intel_dp->link_rate, - intel_dp->lane_count)) - /* Schedule a Hotplug Uevent to userspace to start modeset */ - schedule_work(&intel_connector->modeset_retry_work); - } else { - DRM_ERROR("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", - intel_connector->base.base.id, - intel_connector->base.name, - intel_dp->link_rate, intel_dp->lane_count); - } + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at link rate = %d, lane count = %d", + intel_connector->base.base.id, + intel_connector->base.name, + intel_dp->link_rate, intel_dp->lane_count); + if (!intel_dp_get_link_train_fallback_values(intel_dp, + intel_dp->link_rate, + intel_dp->lane_count)) + /* Schedule a Hotplug Uevent to userspace to start modeset */ + schedule_work(&intel_connector->modeset_retry_work); return; }
This patch fixes the original commit c0cfb10d9e1de49 ("drm/i915/edp: Do not do link training fallback or prune modes on EDP") that causes a blank screen in case of certain eDP panels (Eg: seen on Dell XPS13 9350) where first link training fails and a retraining is required by falling back to lower link rate/lane count. In case of some panels they advertise higher link rate/lane count than whats required for supporting the panel's native mode. But we always link train at highest link rate/lane count for eDP and if that fails we can still fallback to lower link rate/lane count as long as the fallback link BW still fits the native mode to avoid pruning the panel's native mode yet retraining at fallback values to recover from a blank screen. v2: * Send uevent if link failure on eDP unconditionally Cc: Clinton Taylor <clinton.a.taylor@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107489 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105338 Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> Tested-by: Alexander Wilson <alexander.wilson@ncf.edu> --- drivers/gpu/drm/i915/intel_dp.c | 29 +++++++++++++++++++ drivers/gpu/drm/i915/intel_dp_link_training.c | 26 ++++++----------- 2 files changed, 38 insertions(+), 17 deletions(-)