[PATCH 06/14] drm/i915/dp: Send only a single modeset-retry uevent for a commit

[Murthy, Arun R]

> -----Original Message-----
> From: Deak, Imre <imre.deak at intel.com>
> Sent: Thursday, July 25, 2024 5:15 PM
> To: Murthy, Arun R <arun.r.murthy at intel.com>
> Cc: intel-gfx at 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 at intel.com>
> > > Sent: Wednesday, July 24, 2024 4:46 PM
> > > To: Murthy, Arun R <arun.r.murthy at intel.com>
> > > Cc: intel-gfx at 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 at lists.freedesktop.org> On
> > > > > Behalf Of Imre Deak
> > > > > Sent: Monday, July 22, 2024 10:25 PM
> > > > > To: intel-gfx at 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 at 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 at 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
> > > >