diff mbox series

[3/3] drm/i915/dp: Include the time taken by AUX Tx for timeout

Message ID 20240912050552.779356-4-arun.r.murthy@intel.com (mailing list archive)
State New, archived
Headers show
Series Some correction in the DP Link Training dequence | expand

Commit Message

Murthy, Arun R Sept. 12, 2024, 5:05 a.m. UTC 
As per DP spec the timeout for LANE_CHANNEL_EQ_DONE is 400ms. But this
timeout value is exclusively for the Aux RD Interval and excludes the
time consumed for the AUX Tx (i.e reading/writing FFE presets). Add
another 50ms for these AUX Tx to the 400ms timeout.

Signed-off-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_link_training.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kandpal, Suraj Sept. 23, 2024, 6:23 a.m. UTC | #1 
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Arun R
> Murthy
> Sent: Thursday, September 12, 2024 10:36 AM
> To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Srikanth V, NagaVenkata
> <nagavenkata.srikanth.v@intel.com>
> Subject: [PATCH 3/3] drm/i915/dp: Include the time taken by AUX Tx for
> timeout
> 
> As per DP spec the timeout for LANE_CHANNEL_EQ_DONE is 400ms. But this

Adding where in DP spec example dp2.1 section x.x is a good idea

> timeout value is exclusively for the Aux RD Interval and excludes the time
> consumed for the AUX Tx (i.e reading/writing FFE presets). Add another
> 50ms for these AUX Tx to the 400ms timeout.

Is this something we came up with by trial and error or is this also a part of spec

Regards,
Suraj Kandpal
> 
> Signed-off-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_link_training.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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 ca179bed46ad..b6573934c6dd 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -1414,7 +1414,7 @@ intel_dp_128b132b_lane_eq(struct intel_dp
> *intel_dp,
>  	}
> 
>  	/* Time budget for the LANEx_EQ_DONE Sequence */
> -	deadline = jiffies + msecs_to_jiffies_timeout(400);
> +	deadline = jiffies + msecs_to_jiffies_timeout(450);
> 
>  	for (try = 0; try < max_tries; try++) {
>  		fsleep(delay_us);
> --
> 2.25.1
Srikanth V, NagaVenkata Sept. 23, 2024, 6:31 a.m. UTC | #2 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > Arun R Murthy
> > Sent: Thursday, September 12, 2024 10:36 AM
> > To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Srikanth V, NagaVenkata
> > <nagavenkata.srikanth.v@intel.com>
> > Subject: [PATCH 3/3] drm/i915/dp: Include the time taken by AUX Tx for
> > timeout
> >
> > As per DP spec the timeout for LANE_CHANNEL_EQ_DONE is 400ms. But this
> 
> Adding where in DP spec example dp2.1 section x.x is a good idea
> 
Please refer to section 3.5.2.16.1 128b/132b DP LANEx_CHANNEL_EQ_DONE Sequnce
and Figure3-51: 128b/132b DP EQ Time Budget Illustration of DP2.1a spec.

> > timeout value is exclusively for the Aux RD Interval and excludes the
> > time consumed for the AUX Tx (i.e reading/writing FFE presets). Add
> > another 50ms for these AUX Tx to the 400ms timeout.
> 
> Is this something we came up with by trial and error or is this also a part of spec
> 
It's not by trail and erros. Timeout value of 450ms is part of DP.

> Regards,
> Suraj Kandpal
> >
> > Signed-off-by: Srikanth V NagaVenkata
> > <nagavenkata.srikanth.v@intel.com>
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_link_training.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > 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 ca179bed46ad..b6573934c6dd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -1414,7 +1414,7 @@ intel_dp_128b132b_lane_eq(struct intel_dp
> > *intel_dp,
> >  	}
> >
> >  	/* Time budget for the LANEx_EQ_DONE Sequence */
> > -	deadline = jiffies + msecs_to_jiffies_timeout(400);
> > +	deadline = jiffies + msecs_to_jiffies_timeout(450);
> >
> >  	for (try = 0; try < max_tries; try++) {
> >  		fsleep(delay_us);
> > --
> > 2.25.1
Kandpal, Suraj Sept. 23, 2024, 6:58 a.m. UTC | #3 
> -----Original Message-----
> From: Srikanth V, NagaVenkata <nagavenkata.srikanth.v@intel.com>
> Sent: Monday, September 23, 2024 12:02 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>; Murthy, Arun R
> <arun.r.murthy@intel.com>; intel-xe@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: RE: [PATCH 3/3] drm/i915/dp: Include the time taken by AUX Tx for
> timeout
> 
> 
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > > Arun R Murthy
> > > Sent: Thursday, September 12, 2024 10:36 AM
> > > To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Srikanth V,
> > > NagaVenkata <nagavenkata.srikanth.v@intel.com>
> > > Subject: [PATCH 3/3] drm/i915/dp: Include the time taken by AUX Tx
> > > for timeout
> > >
> > > As per DP spec the timeout for LANE_CHANNEL_EQ_DONE is 400ms. But
> > > this
> >
> > Adding where in DP spec example dp2.1 section x.x is a good idea
> >
> Please refer to section 3.5.2.16.1 128b/132b DP LANEx_CHANNEL_EQ_DONE
> Sequnce and Figure3-51: 128b/132b DP EQ Time Budget Illustration of
> DP2.1a spec.
> 
> > > timeout value is exclusively for the Aux RD Interval and excludes
> > > the time consumed for the AUX Tx (i.e reading/writing FFE presets).
> > > Add another 50ms for these AUX Tx to the 400ms timeout.
> >
> > Is this something we came up with by trial and error or is this also a
> > part of spec
> >
> It's not by trail and erros. Timeout value of 450ms is part of DP.

Ahh okay I see it,
After adding the dp spec reference in the commit message this patch
LGTM,
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>

> 
> > Regards,
> > Suraj Kandpal
> > >
> > > Signed-off-by: Srikanth V NagaVenkata
> > > <nagavenkata.srikanth.v@intel.com>
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp_link_training.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > 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 ca179bed46ad..b6573934c6dd 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > @@ -1414,7 +1414,7 @@ intel_dp_128b132b_lane_eq(struct intel_dp
> > > *intel_dp,
> > >  	}
> > >
> > >  	/* Time budget for the LANEx_EQ_DONE Sequence */
> > > -	deadline = jiffies + msecs_to_jiffies_timeout(400);
> > > +	deadline = jiffies + msecs_to_jiffies_timeout(450);
> > >
> > >  	for (try = 0; try < max_tries; try++) {
> > >  		fsleep(delay_us);
> > > --
> > > 2.25.1
Jani Nikula Sept. 23, 2024, 11:45 a.m. UTC | #4 
On Mon, 23 Sep 2024, "Srikanth V, NagaVenkata" <nagavenkata.srikanth.v@intel.com> wrote:
>> > -----Original Message-----
>> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
>> > Arun R Murthy
>> > Sent: Thursday, September 12, 2024 10:36 AM
>> > To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
>> > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Srikanth V, NagaVenkata
>> > <nagavenkata.srikanth.v@intel.com>
>> > Subject: [PATCH 3/3] drm/i915/dp: Include the time taken by AUX Tx for
>> > timeout
>> >
>> > As per DP spec the timeout for LANE_CHANNEL_EQ_DONE is 400ms. But this
>> 
>> Adding where in DP spec example dp2.1 section x.x is a good idea
>> 
> Please refer to section 3.5.2.16.1 128b/132b DP LANEx_CHANNEL_EQ_DONE Sequnce
> and Figure3-51: 128b/132b DP EQ Time Budget Illustration of DP2.1a spec.
>
>> > timeout value is exclusively for the Aux RD Interval and excludes the
>> > time consumed for the AUX Tx (i.e reading/writing FFE presets). Add
>> > another 50ms for these AUX Tx to the 400ms timeout.
>> 
>> Is this something we came up with by trial and error or is this also a part of spec
>> 
> It's not by trail and erros. Timeout value of 450ms is part of DP.

It's a very poorly written spec, and contradicts itself. The only place
where I see 450 ms even mentioned is Figure 3-52. But everywhere else it
seems to say many times 400 ms is the max, without a hint that it would
be exclusive of aux. It does not say anything about the sum of rd
interval being related to the max of 400 ms either.

Regardless, I have no qualms about relaxing the timeout by 50 ms.

With the commit message explaining the above, with references,

Acked-by: Jani Nikula <jani.nikula@intel.com>


>
>> Regards,
>> Suraj Kandpal
>> >
>> > Signed-off-by: Srikanth V NagaVenkata
>> > <nagavenkata.srikanth.v@intel.com>
>> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_dp_link_training.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > 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 ca179bed46ad..b6573934c6dd 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> > @@ -1414,7 +1414,7 @@ intel_dp_128b132b_lane_eq(struct intel_dp
>> > *intel_dp,
>> >  	}
>> >
>> >  	/* Time budget for the LANEx_EQ_DONE Sequence */
>> > -	deadline = jiffies + msecs_to_jiffies_timeout(400);
>> > +	deadline = jiffies + msecs_to_jiffies_timeout(450);
>> >
>> >  	for (try = 0; try < max_tries; try++) {
>> >  		fsleep(delay_us);
>> > --
>> > 2.25.1
>
diff mbox series

Patch

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 ca179bed46ad..b6573934c6dd 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -1414,7 +1414,7 @@  intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
 	}
 
 	/* Time budget for the LANEx_EQ_DONE Sequence */
-	deadline = jiffies + msecs_to_jiffies_timeout(400);
+	deadline = jiffies + msecs_to_jiffies_timeout(450);
 
 	for (try = 0; try < max_tries; try++) {
 		fsleep(delay_us);