| Message ID | 20240722165503.2084999-8-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp_mst: Enable LT fallback for UHBR<->non-UHBR rates | expand |
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre > Deak > Sent: Monday, July 22, 2024 10:25 PM > To: intel-gfx@lists.freedesktop.org > Subject: [PATCH 07/14] drm/i915/dp: Add a separate function to reduce the link > parameters > > A follow-up patch will add an alternative way to reduce the link parameters in > BW order on MST links, prepare for that here. > > Signed-off-by: Imre Deak <imre.deak@intel.com> LGTM, Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > .../drm/i915/display/intel_dp_link_training.c | 39 +++++++++++++++---- > 1 file changed, 31 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > index 58dea87a9fa28..57536ae200b77 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > @@ -1193,6 +1193,36 @@ static int reduce_lane_count(struct intel_dp > *intel_dp, int current_lane_count) > return current_lane_count >> 1; > } > > +static bool reduce_link_params_in_rate_lane_order(struct intel_dp *intel_dp, > + const struct intel_crtc_state > *crtc_state, > + int *new_link_rate, int > *new_lane_count) { > + int link_rate; > + int lane_count; > + > + lane_count = crtc_state->lane_count; > + link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); > + if (link_rate < 0) { > + lane_count = reduce_lane_count(intel_dp, crtc_state- > >lane_count); > + link_rate = intel_dp_max_common_rate(intel_dp); > + } > + > + if (lane_count < 0) > + return false; > + > + *new_link_rate = link_rate; > + *new_lane_count = lane_count; > + > + return true; > +} > + > +static bool reduce_link_params(struct intel_dp *intel_dp, const struct > intel_crtc_state *crtc_state, > + int *new_link_rate, int *new_lane_count) { > + return reduce_link_params_in_rate_lane_order(intel_dp, crtc_state, > + new_link_rate, > new_lane_count); } > + > static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > const struct intel_crtc_state > *crtc_state) { @@ -1206,14 +1236,7 @@ static int > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > return 0; > } > > - new_lane_count = crtc_state->lane_count; > - new_link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); > - if (new_link_rate < 0) { > - new_lane_count = reduce_lane_count(intel_dp, crtc_state- > >lane_count); > - new_link_rate = intel_dp_max_common_rate(intel_dp); > - } > - > - if (new_lane_count < 0) > + if (!reduce_link_params(intel_dp, crtc_state, &new_link_rate, > +&new_lane_count)) > return -1; > > if (intel_dp_is_edp(intel_dp) && > -- > 2.44.2
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre > Deak > Sent: Monday, July 22, 2024 10:25 PM > To: intel-gfx@lists.freedesktop.org > Subject: [PATCH 07/14] drm/i915/dp: Add a separate function to reduce the link > parameters > > A follow-up patch will add an alternative way to reduce the link parameters in > BW order on MST links, prepare for that here. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > .../drm/i915/display/intel_dp_link_training.c | 39 +++++++++++++++---- > 1 file changed, 31 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > index 58dea87a9fa28..57536ae200b77 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > @@ -1193,6 +1193,36 @@ static int reduce_lane_count(struct intel_dp > *intel_dp, int current_lane_count) > return current_lane_count >> 1; > } > > +static bool reduce_link_params_in_rate_lane_order(struct intel_dp *intel_dp, > + const struct intel_crtc_state > *crtc_state, > + int *new_link_rate, int > *new_lane_count) { > + int link_rate; > + int lane_count; > + > + lane_count = crtc_state->lane_count; > + link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); > + if (link_rate < 0) { > + lane_count = reduce_lane_count(intel_dp, crtc_state- > >lane_count); > + link_rate = intel_dp_max_common_rate(intel_dp); > + } > + On link training failure reducing link rate or lane count is not linear. Sometime we fall from uhbr to hbr and then again with uhbr with lane reduction. So would it be better to have a table/list for the fallback link rate/lane count. Thanks and Regards, Arun R Murthy -------------------- > + if (lane_count < 0) > + return false; > + > + *new_link_rate = link_rate; > + *new_lane_count = lane_count; > + > + return true; > +} > + > +static bool reduce_link_params(struct intel_dp *intel_dp, const struct > intel_crtc_state *crtc_state, > + int *new_link_rate, int *new_lane_count) { > + return reduce_link_params_in_rate_lane_order(intel_dp, crtc_state, > + new_link_rate, > new_lane_count); } > + > static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > const struct intel_crtc_state > *crtc_state) { @@ -1206,14 +1236,7 @@ static int > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > return 0; > } > > - new_lane_count = crtc_state->lane_count; > - new_link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); > - if (new_link_rate < 0) { > - new_lane_count = reduce_lane_count(intel_dp, crtc_state- > >lane_count); > - new_link_rate = intel_dp_max_common_rate(intel_dp); > - } > - > - if (new_lane_count < 0) > + if (!reduce_link_params(intel_dp, crtc_state, &new_link_rate, > +&new_lane_count)) > return -1; > > if (intel_dp_is_edp(intel_dp) && > -- > 2.44.2
On Wed, Jul 24, 2024 at 07:55:03AM +0300, Murthy, Arun R wrote: > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre > > Deak > > Sent: Monday, July 22, 2024 10:25 PM > > To: intel-gfx@lists.freedesktop.org > > Subject: [PATCH 07/14] drm/i915/dp: Add a separate function to reduce the link > > parameters > > > > A follow-up patch will add an alternative way to reduce the link parameters in > > BW order on MST links, prepare for that here. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > .../drm/i915/display/intel_dp_link_training.c | 39 +++++++++++++++---- > > 1 file changed, 31 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > index 58dea87a9fa28..57536ae200b77 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > @@ -1193,6 +1193,36 @@ static int reduce_lane_count(struct intel_dp > > *intel_dp, int current_lane_count) > > return current_lane_count >> 1; > > } > > > > +static bool reduce_link_params_in_rate_lane_order(struct intel_dp *intel_dp, > > + const struct intel_crtc_state > > *crtc_state, > > + int *new_link_rate, int > > *new_lane_count) { > > + int link_rate; > > + int lane_count; > > + > > + lane_count = crtc_state->lane_count; > > + link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); > > + if (link_rate < 0) { > > + lane_count = reduce_lane_count(intel_dp, crtc_state- > > >lane_count); > > + link_rate = intel_dp_max_common_rate(intel_dp); > > + } > > + > > On link training failure reducing link rate or lane count is not > linear. Sometime we fall from uhbr to hbr and then again with uhbr > with lane reduction. So would it be better to have a table/list for > the fallback link rate/lane count. This patch is meant to to keep the current way of reducing the rate and lane count, just preparing for a follow-up change that adds the alternetive BW order fallback logic for MST. I think later SST would need to be switched to the logic as well, for now I didn't want to change this. > > Thanks and Regards, > Arun R Murthy > -------------------- > > + if (lane_count < 0) > > + return false; > > + > > + *new_link_rate = link_rate; > > + *new_lane_count = lane_count; > > + > > + return true; > > +} > > + > > +static bool reduce_link_params(struct intel_dp *intel_dp, const struct > > intel_crtc_state *crtc_state, > > + int *new_link_rate, int *new_lane_count) { > > + return reduce_link_params_in_rate_lane_order(intel_dp, crtc_state, > > + new_link_rate, > > new_lane_count); } > > + > > static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > const struct intel_crtc_state > > *crtc_state) { @@ -1206,14 +1236,7 @@ static int > > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > return 0; > > } > > > > - new_lane_count = crtc_state->lane_count; > > - new_link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); > > - if (new_link_rate < 0) { > > - new_lane_count = reduce_lane_count(intel_dp, crtc_state- > > >lane_count); > > - new_link_rate = intel_dp_max_common_rate(intel_dp); > > - } > > - > > - if (new_lane_count < 0) > > + if (!reduce_link_params(intel_dp, crtc_state, &new_link_rate, > > +&new_lane_count)) > > return -1; > > > > if (intel_dp_is_edp(intel_dp) && > > -- > > 2.44.2 >
> -----Original Message----- > From: Deak, Imre <imre.deak@intel.com> > Sent: Wednesday, July 24, 2024 4:50 PM > To: Murthy, Arun R <arun.r.murthy@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 07/14] drm/i915/dp: Add a separate function to reduce the > link parameters > > On Wed, Jul 24, 2024 at 07:55:03AM +0300, Murthy, Arun R wrote: > > > -----Original Message----- > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf > > > Of Imre Deak > > > Sent: Monday, July 22, 2024 10:25 PM > > > To: intel-gfx@lists.freedesktop.org > > > Subject: [PATCH 07/14] drm/i915/dp: Add a separate function to > > > reduce the link parameters > > > > > > A follow-up patch will add an alternative way to reduce the link > > > parameters in BW order on MST links, prepare for that here. > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > .../drm/i915/display/intel_dp_link_training.c | 39 > > > +++++++++++++++---- > > > 1 file changed, 31 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > index 58dea87a9fa28..57536ae200b77 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > @@ -1193,6 +1193,36 @@ static int reduce_lane_count(struct intel_dp > > > *intel_dp, int current_lane_count) > > > return current_lane_count >> 1; } > > > > > > +static bool reduce_link_params_in_rate_lane_order(struct intel_dp > *intel_dp, > > > + const struct > > > +intel_crtc_state > > > *crtc_state, > > > + int *new_link_rate, > > > + int > > > *new_lane_count) { > > > + int link_rate; > > > + int lane_count; > > > + > > > + lane_count = crtc_state->lane_count; > > > + link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); > > > + if (link_rate < 0) { > > > + lane_count = reduce_lane_count(intel_dp, crtc_state- > > > >lane_count); > > > + link_rate = intel_dp_max_common_rate(intel_dp); > > > + } > > > + > > > > On link training failure reducing link rate or lane count is not > > linear. Sometime we fall from uhbr to hbr and then again with uhbr > > with lane reduction. So would it be better to have a table/list for > > the fallback link rate/lane count. > > This patch is meant to to keep the current way of reducing the rate and lane > count, just preparing for a follow-up change that adds the alternetive BW order > fallback logic for MST. I think later SST would need to be switched to the logic as > well, for now I didn't want to change this. > This series enables fallback for 128b/132b as well and the fallback linkrate and lanecount values for them are not in linear reducing manner. Can we have a TODO in this function about this? Thanks and Regards, Arun R Murthy -------------------- > > > > Thanks and Regards, > > Arun R Murthy > > -------------------- > > > + if (lane_count < 0) > > > + return false; > > > + > > > + *new_link_rate = link_rate; > > > + *new_lane_count = lane_count; > > > + > > > + return true; > > > +} > > > + > > > +static bool reduce_link_params(struct intel_dp *intel_dp, const > > > +struct > > > intel_crtc_state *crtc_state, > > > + int *new_link_rate, int *new_lane_count) { > > > + return reduce_link_params_in_rate_lane_order(intel_dp, crtc_state, > > > + new_link_rate, > > > new_lane_count); } > > > + > > > static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > > const struct > > > intel_crtc_state > > > *crtc_state) { @@ -1206,14 +1236,7 @@ static int > > > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > > return 0; > > > } > > > > > > - new_lane_count = crtc_state->lane_count; > > > - new_link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); > > > - if (new_link_rate < 0) { > > > - new_lane_count = reduce_lane_count(intel_dp, crtc_state- > > > >lane_count); > > > - new_link_rate = intel_dp_max_common_rate(intel_dp); > > > - } > > > - > > > - if (new_lane_count < 0) > > > + if (!reduce_link_params(intel_dp, crtc_state, &new_link_rate, > > > +&new_lane_count)) > > > return -1; > > > > > > if (intel_dp_is_edp(intel_dp) && > > > -- > > > 2.44.2 > >
On Thu, Jul 25, 2024 at 06:20:23AM +0300, Murthy, Arun R wrote: > > -----Original Message----- > > From: Deak, Imre <imre.deak@intel.com> > > Sent: Wednesday, July 24, 2024 4:50 PM > > To: Murthy, Arun R <arun.r.murthy@intel.com> > > Cc: intel-gfx@lists.freedesktop.org > > Subject: Re: [PATCH 07/14] drm/i915/dp: Add a separate function to reduce the > > link parameters > > > > On Wed, Jul 24, 2024 at 07:55:03AM +0300, Murthy, Arun R wrote: > > > > -----Original Message----- > > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf > > > > Of Imre Deak > > > > Sent: Monday, July 22, 2024 10:25 PM > > > > To: intel-gfx@lists.freedesktop.org > > > > Subject: [PATCH 07/14] drm/i915/dp: Add a separate function to > > > > reduce the link parameters > > > > > > > > A follow-up patch will add an alternative way to reduce the link > > > > parameters in BW order on MST links, prepare for that here. > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > .../drm/i915/display/intel_dp_link_training.c | 39 > > > > +++++++++++++++---- > > > > 1 file changed, 31 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > > b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > > index 58dea87a9fa28..57536ae200b77 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > > @@ -1193,6 +1193,36 @@ static int reduce_lane_count(struct intel_dp > > > > *intel_dp, int current_lane_count) > > > > return current_lane_count >> 1; } > > > > > > > > +static bool reduce_link_params_in_rate_lane_order(struct intel_dp > > *intel_dp, > > > > + const struct > > > > +intel_crtc_state > > > > *crtc_state, > > > > + int *new_link_rate, > > > > + int > > > > *new_lane_count) { > > > > + int link_rate; > > > > + int lane_count; > > > > + > > > > + lane_count = crtc_state->lane_count; > > > > + link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); > > > > + if (link_rate < 0) { > > > > + lane_count = reduce_lane_count(intel_dp, crtc_state- > > > > >lane_count); > > > > + link_rate = intel_dp_max_common_rate(intel_dp); > > > > + } > > > > + > > > > > > On link training failure reducing link rate or lane count is not > > > linear. Sometime we fall from uhbr to hbr and then again with uhbr > > > with lane reduction. So would it be better to have a table/list for > > > the fallback link rate/lane count. > > > > This patch is meant to to keep the current way of reducing the rate and lane > > count, just preparing for a follow-up change that adds the alternetive BW order > > fallback logic for MST. I think later SST would need to be switched to the logic as > > well, for now I didn't want to change this. > > > This series enables fallback for 128b/132b as well and the fallback > linkrate and lanecount values for them are not in linear reducing > manner. Can we have a TODO in this function about this? Not sure what you mean. I can add a TODO comment to change the fallback logic on SST links to happen the same way as on MST links, decreasing the link's BW at each step. > Thanks and Regards, > Arun R Murthy > -------------------- > > > > > > Thanks and Regards, > > > Arun R Murthy > > > -------------------- > > > > + if (lane_count < 0) > > > > + return false; > > > > + > > > > + *new_link_rate = link_rate; > > > > + *new_lane_count = lane_count; > > > > + > > > > + return true; > > > > +} > > > > + > > > > +static bool reduce_link_params(struct intel_dp *intel_dp, const > > > > +struct > > > > intel_crtc_state *crtc_state, > > > > + int *new_link_rate, int *new_lane_count) { > > > > + return reduce_link_params_in_rate_lane_order(intel_dp, crtc_state, > > > > + new_link_rate, > > > > new_lane_count); } > > > > + > > > > static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > > > const struct > > > > intel_crtc_state > > > > *crtc_state) { @@ -1206,14 +1236,7 @@ static int > > > > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > > > return 0; > > > > } > > > > > > > > - new_lane_count = crtc_state->lane_count; > > > > - new_link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); > > > > - if (new_link_rate < 0) { > > > > - new_lane_count = reduce_lane_count(intel_dp, crtc_state- > > > > >lane_count); > > > > - new_link_rate = intel_dp_max_common_rate(intel_dp); > > > > - } > > > > - > > > > - if (new_lane_count < 0) > > > > + if (!reduce_link_params(intel_dp, crtc_state, &new_link_rate, > > > > +&new_lane_count)) > > > > return -1; > > > > > > > > if (intel_dp_is_edp(intel_dp) && > > > > -- > > > > 2.44.2 > > >
diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c index 58dea87a9fa28..57536ae200b77 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -1193,6 +1193,36 @@ static int reduce_lane_count(struct intel_dp *intel_dp, int current_lane_count) return current_lane_count >> 1; } +static bool reduce_link_params_in_rate_lane_order(struct intel_dp *intel_dp, + const struct intel_crtc_state *crtc_state, + int *new_link_rate, int *new_lane_count) +{ + int link_rate; + int lane_count; + + lane_count = crtc_state->lane_count; + link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); + if (link_rate < 0) { + lane_count = reduce_lane_count(intel_dp, crtc_state->lane_count); + link_rate = intel_dp_max_common_rate(intel_dp); + } + + if (lane_count < 0) + return false; + + *new_link_rate = link_rate; + *new_lane_count = lane_count; + + return true; +} + +static bool reduce_link_params(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state, + int *new_link_rate, int *new_lane_count) +{ + return reduce_link_params_in_rate_lane_order(intel_dp, crtc_state, + new_link_rate, new_lane_count); +} + static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state) { @@ -1206,14 +1236,7 @@ static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, return 0; } - new_lane_count = crtc_state->lane_count; - new_link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock); - if (new_link_rate < 0) { - new_lane_count = reduce_lane_count(intel_dp, crtc_state->lane_count); - new_link_rate = intel_dp_max_common_rate(intel_dp); - } - - if (new_lane_count < 0) + if (!reduce_link_params(intel_dp, crtc_state, &new_link_rate, &new_lane_count)) return -1; if (intel_dp_is_edp(intel_dp) &&
A follow-up patch will add an alternative way to reduce the link parameters in BW order on MST links, prepare for that here. Signed-off-by: Imre Deak <imre.deak@intel.com> --- .../drm/i915/display/intel_dp_link_training.c | 39 +++++++++++++++---- 1 file changed, 31 insertions(+), 8 deletions(-)