On Thu, May 17, 2018 at 10:28:02AM +0300, Jani Nikula wrote:
> On Wed, 16 May 2018, Manasi Navare <manasi.d.navare@xxxxxxxxx> 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.
>
> What eDP revision is the faulty panel? Does [1] help for that? Then we'd
> not need this.
>
This is a eDP 1.3 panel and so it wont pick the fast and narrow approach
in [1]. So our assumption that eDP 1.3 and earlier panels support only a single
clock and lane configuration that correspond to the native resolution of
the panel doesnt hold good here.
> I think this also ties in with the alternate/downclock mode. It fails
> now and should be reverted [2]. However, if we know the panel has a
> valid mode with lower refresh rate, we might have a mode that actually
> fits a link with reduced bandwidth.
>
> IMO we need [1], [2], and patches to disconnect downclock mode from drrs
> support (so we/user can choose the downclock mode independent of drrs),
> and finally eDP link fallback selection based on if there's a downclock
> mode and if it fits a link with reduced link rate or lane count.
>
Yes so the intel_find_panel_downclock() needs to be called independently
from intel_dp_drrs_init() right?
After that is done then my function in this patch can also check if the
reduced fallback BW can support downclock mode and if so then allow
link train fallback for eDP.
Does this sound like a correct approach?
Manasi
> BR,
> Jani.
>
>
> [1] http://patchwork.freedesktop.org/patch/msgid/20180509071321.28563-1-jani.nikula@xxxxxxxxx
> [2] http://patchwork.freedesktop.org/patch/msgid/20180516080110.22770-1-jani.nikula@xxxxxxxxx
>
>
>
> >
> > Cc: Clinton Taylor <clinton.a.taylor@xxxxxxxxx>
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_dp_link_training.c | 26 +++++++++-----------------
> > 2 files changed, 34 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 2cc58596..7f7202a 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -387,6 +387,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)
> > {
> > @@ -396,9 +411,19 @@ 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))
> > + return -1;
> > 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))
> > + return -1;
> > 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 3fcaa98..6673975 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -335,22 +335,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;
> > }
>
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
