| Message ID | 20240722165503.2084999-7-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 06/14] drm/i915/dp: Send only a single modeset-retry uevent > for a commit > > There are multiple failure cases a modeset-retry uevent can be sent for a link > (TBT tunnel BW allocation failure, unrecoverable link training failure), a follow- > up patch adding the handling for a new case where the DP MST payload > allocation fails. The uevent is the same in all cases, sent to all the connectors on > the link, so in case of multiple failures there is no point in sending a separate > uevent for each failure; prevent this, sending only a single modeset-retry > uevent for a commit. > Is an exit condition required with some 'x' retry so that this retry doesn't end up in an infinite loop. For link training failure the link rate/lane count is reduced and when it reaches the least can exit, but for BW allocation failures/payload failure this may not be the case. Thanks and Regards, Arun R Murthy -------------------- > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > drivers/gpu/drm/i915/display/intel_dp.c | 6 ++++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > b/drivers/gpu/drm/i915/display/intel_display_types.h > index a9d2acdc51a4a..3501125c55158 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -1754,6 +1754,7 @@ struct intel_dp { > u8 lane_count; > u8 sink_count; > bool link_trained; > + bool needs_modeset_retry; > bool use_max_params; > u8 dpcd[DP_RECEIVER_CAP_SIZE]; > u8 psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE]; > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 421e970b3c180..0882dddd97206 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -2876,6 +2876,11 @@ intel_dp_queue_modeset_retry_for_link(struct > intel_atomic_state *state, > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > int i; > > + if (intel_dp->needs_modeset_retry) > + return; > + > + intel_dp->needs_modeset_retry = true; > + > if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) { > intel_dp_queue_modeset_retry_work(intel_dp- > >attached_connector); > > @@ -3009,6 +3014,7 @@ void intel_dp_set_link_params(struct intel_dp > *intel_dp, { > memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > intel_dp->link_trained = false; > + intel_dp->needs_modeset_retry = false; > intel_dp->link_rate = link_rate; > intel_dp->lane_count = lane_count; > } > -- > 2.44.2
On Wed, Jul 24, 2024 at 07:29:33AM +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 06/14] drm/i915/dp: Send only a single modeset-retry uevent > > for a commit > > > > There are multiple failure cases a modeset-retry uevent can be sent for a link > > (TBT tunnel BW allocation failure, unrecoverable link training failure), a follow- > > up patch adding the handling for a new case where the DP MST payload > > allocation fails. The uevent is the same in all cases, sent to all the connectors on > > the link, so in case of multiple failures there is no point in sending a separate > > uevent for each failure; prevent this, sending only a single modeset-retry > > uevent for a commit. > > > Is an exit condition required with some 'x' retry so that this retry > doesn't end up in an infinite loop. For link training failure the > link rate/lane count is reduced and when it reaches the least can > exit, but for BW allocation failures/payload failure this may not be > the case. This is an error condition the driver reports (asynchronously) if a modeset request by userspace/client failed. It would be incorrect not to report this error, leaving the output in a blank, enabled state. I think that userspace/client should handle such failures - in the above case a buggy sink - by disabling the output. > Thanks and Regards, > Arun R Murthy > -------------------- > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > > drivers/gpu/drm/i915/display/intel_dp.c | 6 ++++++ > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index a9d2acdc51a4a..3501125c55158 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1754,6 +1754,7 @@ struct intel_dp { > > u8 lane_count; > > u8 sink_count; > > bool link_trained; > > + bool needs_modeset_retry; > > bool use_max_params; > > u8 dpcd[DP_RECEIVER_CAP_SIZE]; > > u8 psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE]; > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > b/drivers/gpu/drm/i915/display/intel_dp.c > > index 421e970b3c180..0882dddd97206 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -2876,6 +2876,11 @@ intel_dp_queue_modeset_retry_for_link(struct > > intel_atomic_state *state, > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > int i; > > > > + if (intel_dp->needs_modeset_retry) > > + return; > > + > > + intel_dp->needs_modeset_retry = true; > > + > > if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) { > > intel_dp_queue_modeset_retry_work(intel_dp- > > >attached_connector); > > > > @@ -3009,6 +3014,7 @@ void intel_dp_set_link_params(struct intel_dp > > *intel_dp, { > > memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > > intel_dp->link_trained = false; > > + intel_dp->needs_modeset_retry = false; > > intel_dp->link_rate = link_rate; > > intel_dp->lane_count = lane_count; > > } > > -- > > 2.44.2 >
> -----Original Message----- > From: Deak, Imre <imre.deak@intel.com> > Sent: Wednesday, July 24, 2024 4:46 PM > To: Murthy, Arun R <arun.r.murthy@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 06/14] drm/i915/dp: Send only a single modeset-retry > uevent for a commit > > On Wed, Jul 24, 2024 at 07:29:33AM +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 06/14] drm/i915/dp: Send only a single modeset-retry > > > uevent for a commit > > > > > > There are multiple failure cases a modeset-retry uevent can be sent > > > for a link (TBT tunnel BW allocation failure, unrecoverable link > > > training failure), a follow- up patch adding the handling for a new > > > case where the DP MST payload allocation fails. The uevent is the > > > same in all cases, sent to all the connectors on the link, so in > > > case of multiple failures there is no point in sending a separate > > > uevent for each failure; prevent this, sending only a single modeset-retry > uevent for a commit. > > > > > Is an exit condition required with some 'x' retry so that this retry > > doesn't end up in an infinite loop. For link training failure the > > link rate/lane count is reduced and when it reaches the least can > > exit, but for BW allocation failures/payload failure this may not be > > the case. > > This is an error condition the driver reports (asynchronously) if a modeset > request by userspace/client failed. It would be incorrect not to report this error, > leaving the output in a blank, enabled state. > > I think that userspace/client should handle such failures - in the above case a > buggy sink - by disabling the output. > If user space doesn't then I think we end up in an infinite loop in KMD. So would it make some sense to have some exit condition in KMD. Thanks and Regards, Arun R Murthy -------------------- > > Thanks and Regards, > > Arun R Murthy > > -------------------- > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > > > drivers/gpu/drm/i915/display/intel_dp.c | 6 ++++++ > > > 2 files changed, 7 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index a9d2acdc51a4a..3501125c55158 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -1754,6 +1754,7 @@ struct intel_dp { > > > u8 lane_count; > > > u8 sink_count; > > > bool link_trained; > > > + bool needs_modeset_retry; > > > bool use_max_params; > > > u8 dpcd[DP_RECEIVER_CAP_SIZE]; > > > u8 psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE]; > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > index 421e970b3c180..0882dddd97206 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -2876,6 +2876,11 @@ intel_dp_queue_modeset_retry_for_link(struct > > > intel_atomic_state *state, > > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > > int i; > > > > > > + if (intel_dp->needs_modeset_retry) > > > + return; > > > + > > > + intel_dp->needs_modeset_retry = true; > > > + > > > if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) { > > > intel_dp_queue_modeset_retry_work(intel_dp- > > > >attached_connector); > > > > > > @@ -3009,6 +3014,7 @@ void intel_dp_set_link_params(struct intel_dp > > > *intel_dp, { > > > memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > > > intel_dp->link_trained = false; > > > + intel_dp->needs_modeset_retry = false; > > > intel_dp->link_rate = link_rate; > > > intel_dp->lane_count = lane_count; } > > > -- > > > 2.44.2 > >
On Thu, Jul 25, 2024 at 06:16:03AM +0300, Murthy, Arun R wrote: > > -----Original Message----- > > From: Deak, Imre <imre.deak@intel.com> > > Sent: Wednesday, July 24, 2024 4:46 PM > > To: Murthy, Arun R <arun.r.murthy@intel.com> > > Cc: intel-gfx@lists.freedesktop.org > > Subject: Re: [PATCH 06/14] drm/i915/dp: Send only a single modeset-retry > > uevent for a commit > > > > On Wed, Jul 24, 2024 at 07:29:33AM +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 06/14] drm/i915/dp: Send only a single modeset-retry > > > > uevent for a commit > > > > > > > > There are multiple failure cases a modeset-retry uevent can be sent > > > > for a link (TBT tunnel BW allocation failure, unrecoverable link > > > > training failure), a follow- up patch adding the handling for a new > > > > case where the DP MST payload allocation fails. The uevent is the > > > > same in all cases, sent to all the connectors on the link, so in > > > > case of multiple failures there is no point in sending a separate > > > > uevent for each failure; prevent this, sending only a single modeset-retry > > uevent for a commit. > > > > > > > Is an exit condition required with some 'x' retry so that this retry > > > doesn't end up in an infinite loop. For link training failure the > > > link rate/lane count is reduced and when it reaches the least can > > > exit, but for BW allocation failures/payload failure this may not be > > > the case. > > > > This is an error condition the driver reports (asynchronously) if a modeset > > request by userspace/client failed. It would be incorrect not to report this error, > > leaving the output in a blank, enabled state. > > > > I think that userspace/client should handle such failures - in the above case a > > buggy sink - by disabling the output. > > > If user space doesn't then I think we end up in an infinite loop in > KMD. So would it make some sense to have some exit condition in KMD. It would be a bug in userspace to keep modesetting in an infinite loop, instead of disabling the output; the same could happen already after LT failures. Adding a workaround for such users wouldn't be simply not returning any error to the user and is not the topic of this patchset. > Thanks and Regards, > Arun R Murthy > -------------------- > > > Thanks and Regards, > > > Arun R Murthy > > > -------------------- > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > > > > drivers/gpu/drm/i915/display/intel_dp.c | 6 ++++++ > > > > 2 files changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > index a9d2acdc51a4a..3501125c55158 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > @@ -1754,6 +1754,7 @@ struct intel_dp { > > > > u8 lane_count; > > > > u8 sink_count; > > > > bool link_trained; > > > > + bool needs_modeset_retry; > > > > bool use_max_params; > > > > u8 dpcd[DP_RECEIVER_CAP_SIZE]; > > > > u8 psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE]; > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > index 421e970b3c180..0882dddd97206 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > @@ -2876,6 +2876,11 @@ intel_dp_queue_modeset_retry_for_link(struct > > > > intel_atomic_state *state, > > > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > > > int i; > > > > > > > > + if (intel_dp->needs_modeset_retry) > > > > + return; > > > > + > > > > + intel_dp->needs_modeset_retry = true; > > > > + > > > > if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) { > > > > intel_dp_queue_modeset_retry_work(intel_dp- > > > > >attached_connector); > > > > > > > > @@ -3009,6 +3014,7 @@ void intel_dp_set_link_params(struct intel_dp > > > > *intel_dp, { > > > > memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > > > > intel_dp->link_trained = false; > > > > + intel_dp->needs_modeset_retry = false; > > > > intel_dp->link_rate = link_rate; > > > > intel_dp->lane_count = lane_count; } > > > > -- > > > > 2.44.2 > > >
> -----Original Message----- > From: Deak, Imre <imre.deak@intel.com> > Sent: Thursday, July 25, 2024 5:15 PM > To: Murthy, Arun R <arun.r.murthy@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 06/14] drm/i915/dp: Send only a single modeset-retry > uevent for a commit > > On Thu, Jul 25, 2024 at 06:16:03AM +0300, Murthy, Arun R wrote: > > > -----Original Message----- > > > From: Deak, Imre <imre.deak@intel.com> > > > Sent: Wednesday, July 24, 2024 4:46 PM > > > To: Murthy, Arun R <arun.r.murthy@intel.com> > > > Cc: intel-gfx@lists.freedesktop.org > > > Subject: Re: [PATCH 06/14] drm/i915/dp: Send only a single > > > modeset-retry uevent for a commit > > > > > > On Wed, Jul 24, 2024 at 07:29:33AM +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 06/14] drm/i915/dp: Send only a single > > > > > modeset-retry uevent for a commit > > > > > > > > > > There are multiple failure cases a modeset-retry uevent can be > > > > > sent for a link (TBT tunnel BW allocation failure, unrecoverable > > > > > link training failure), a follow- up patch adding the handling > > > > > for a new case where the DP MST payload allocation fails. The > > > > > uevent is the same in all cases, sent to all the connectors on > > > > > the link, so in case of multiple failures there is no point in > > > > > sending a separate uevent for each failure; prevent this, > > > > > sending only a single modeset-retry > > > uevent for a commit. > > > > > > > > > Is an exit condition required with some 'x' retry so that this > > > > retry doesn't end up in an infinite loop. For link training > > > > failure the link rate/lane count is reduced and when it reaches > > > > the least can exit, but for BW allocation failures/payload failure > > > > this may not be the case. > > > > > > This is an error condition the driver reports (asynchronously) if a > > > modeset request by userspace/client failed. It would be incorrect > > > not to report this error, leaving the output in a blank, enabled state. > > > > > > I think that userspace/client should handle such failures - in the > > > above case a buggy sink - by disabling the output. > > > > > If user space doesn't then I think we end up in an infinite loop in > > KMD. So would it make some sense to have some exit condition in KMD. > > It would be a bug in userspace to keep modesetting in an infinite loop, instead > of disabling the output; the same could happen already after LT failures. Adding > a workaround for such users wouldn't be simply not returning any error to the > user and is not the topic of this patchset. > Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com> Thanks and Regards, Arun R Murthy ------------------- > > Thanks and Regards, > > Arun R Murthy > > -------------------- > > > > Thanks and Regards, > > > > Arun R Murthy > > > > -------------------- > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 6 ++++++ > > > > > 2 files changed, 7 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > index a9d2acdc51a4a..3501125c55158 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > @@ -1754,6 +1754,7 @@ struct intel_dp { > > > > > u8 lane_count; > > > > > u8 sink_count; > > > > > bool link_trained; > > > > > + bool needs_modeset_retry; > > > > > bool use_max_params; > > > > > u8 dpcd[DP_RECEIVER_CAP_SIZE]; > > > > > u8 psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE]; > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > index 421e970b3c180..0882dddd97206 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > @@ -2876,6 +2876,11 @@ > > > > > intel_dp_queue_modeset_retry_for_link(struct > > > > > intel_atomic_state *state, > > > > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > > > > int i; > > > > > > > > > > + if (intel_dp->needs_modeset_retry) > > > > > + return; > > > > > + > > > > > + intel_dp->needs_modeset_retry = true; > > > > > + > > > > > if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) { > > > > > intel_dp_queue_modeset_retry_work(intel_dp- > > > > > >attached_connector); > > > > > > > > > > @@ -3009,6 +3014,7 @@ void intel_dp_set_link_params(struct > > > > > intel_dp *intel_dp, { > > > > > memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > > > > > intel_dp->link_trained = false; > > > > > + intel_dp->needs_modeset_retry = false; > > > > > intel_dp->link_rate = link_rate; > > > > > intel_dp->lane_count = lane_count; } > > > > > -- > > > > > 2.44.2 > > > >
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index a9d2acdc51a4a..3501125c55158 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1754,6 +1754,7 @@ struct intel_dp { u8 lane_count; u8 sink_count; bool link_trained; + bool needs_modeset_retry; bool use_max_params; u8 dpcd[DP_RECEIVER_CAP_SIZE]; u8 psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE]; diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 421e970b3c180..0882dddd97206 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -2876,6 +2876,11 @@ intel_dp_queue_modeset_retry_for_link(struct intel_atomic_state *state, struct intel_dp *intel_dp = enc_to_intel_dp(encoder); int i; + if (intel_dp->needs_modeset_retry) + return; + + intel_dp->needs_modeset_retry = true; + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) { intel_dp_queue_modeset_retry_work(intel_dp->attached_connector); @@ -3009,6 +3014,7 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp, { memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); intel_dp->link_trained = false; + intel_dp->needs_modeset_retry = false; intel_dp->link_rate = link_rate; intel_dp->lane_count = lane_count; }
There are multiple failure cases a modeset-retry uevent can be sent for a link (TBT tunnel BW allocation failure, unrecoverable link training failure), a follow-up patch adding the handling for a new case where the DP MST payload allocation fails. The uevent is the same in all cases, sent to all the connectors on the link, so in case of multiple failures there is no point in sending a separate uevent for each failure; prevent this, sending only a single modeset-retry uevent for a commit. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_display_types.h | 1 + drivers/gpu/drm/i915/display/intel_dp.c | 6 ++++++ 2 files changed, 7 insertions(+)