diff mbox series

[1/3] drm/i915/dp: use fsleep instead of usleep_rage for LT

Message ID 20240912050552.779356-2-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 
Aux RD Interval value depends on the value read from the dpcd register
which is updated from the sink device use flseep thereby we adhere to
the Documentation/timers/timers-howto.rst

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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Jani Nikula Sept. 12, 2024, 9:01 a.m. UTC | #1 
On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> Aux RD Interval value depends on the value read from the dpcd register
> which is updated from the sink device use flseep thereby we adhere to
> the Documentation/timers/timers-howto.rst

Please explain why instead of just referencing a file.

BR,
Jani.

> 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 | 6 +++---
>  1 file changed, 3 insertions(+), 3 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 397cc4ebae52..f41b69840ad9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -898,7 +898,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp,
>  
>  	voltage_tries = 1;
>  	for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
> -		usleep_range(delay_us, 2 * delay_us);
> +		fsleep(delay_us);
>  
>  		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
>  						     link_status) < 0) {
> @@ -1040,7 +1040,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp,
>  	}
>  
>  	for (tries = 0; tries < 5; tries++) {
> -		usleep_range(delay_us, 2 * delay_us);
> +		fsleep(delay_us);
>  
>  		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
>  						     link_status) < 0) {
> @@ -1417,7 +1417,7 @@ intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
>  	deadline = jiffies + msecs_to_jiffies_timeout(400);
>  
>  	for (try = 0; try < max_tries; try++) {
> -		usleep_range(delay_us, 2 * delay_us);
> +		fsleep(delay_us);
>  
>  		/*
>  		 * The delay may get updated. The transmitter shall read the
Jani Nikula Sept. 12, 2024, 9:05 a.m. UTC | #2 
On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> Aux RD Interval value depends on the value read from the dpcd register
> which is updated from the sink device use flseep thereby we adhere to
> the Documentation/timers/timers-howto.rst
>
> Signed-off-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>

Why do all the patches have two Signed-off-by's?

BR,
Jani.

> ---
>  drivers/gpu/drm/i915/display/intel_dp_link_training.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 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 397cc4ebae52..f41b69840ad9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -898,7 +898,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp,
>  
>  	voltage_tries = 1;
>  	for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
> -		usleep_range(delay_us, 2 * delay_us);
> +		fsleep(delay_us);
>  
>  		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
>  						     link_status) < 0) {
> @@ -1040,7 +1040,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp,
>  	}
>  
>  	for (tries = 0; tries < 5; tries++) {
> -		usleep_range(delay_us, 2 * delay_us);
> +		fsleep(delay_us);
>  
>  		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
>  						     link_status) < 0) {
> @@ -1417,7 +1417,7 @@ intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
>  	deadline = jiffies + msecs_to_jiffies_timeout(400);
>  
>  	for (try = 0; try < max_tries; try++) {
> -		usleep_range(delay_us, 2 * delay_us);
> +		fsleep(delay_us);
>  
>  		/*
>  		 * The delay may get updated. The transmitter shall read the
Murthy, Arun R Sept. 12, 2024, 10:04 a.m. UTC | #3 
> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Thursday, September 12, 2024 2:32 PM
> To: 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>; Srikanth V, NagaVenkata
> <nagavenkata.srikanth.v@intel.com>
> Subject: Re: [PATCH 1/3] drm/i915/dp: use fsleep instead of usleep_rage for LT
> 
> On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> > Aux RD Interval value depends on the value read from the dpcd register
> > which is updated from the sink device use flseep thereby we adhere to
> > the Documentation/timers/timers-howto.rst
> 
> Please explain why instead of just referencing a file.

Sleeping for < 10us use udelay, for 10us to 20ms use usleep_range and for > 10ms use msleep.
fsleep() will call the particular api based on the above condition.

Thanks and Regards,
Arun R Murthy
--------------------
Murthy, Arun R Sept. 12, 2024, 10:57 a.m. UTC | #4 
> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Thursday, September 12, 2024 2:36 PM
> To: 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>; Srikanth V, NagaVenkata
> <nagavenkata.srikanth.v@intel.com>
> Subject: Re: [PATCH 1/3] drm/i915/dp: use fsleep instead of usleep_rage for LT
> 
> On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> > Aux RD Interval value depends on the value read from the dpcd register
> > which is updated from the sink device use flseep thereby we adhere to
> > the Documentation/timers/timers-howto.rst
> >
> > Signed-off-by: Srikanth V NagaVenkata
> > <nagavenkata.srikanth.v@intel.com>
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> 
> Why do all the patches have two Signed-off-by's?
> 
Findings are from Srikanth so giving credits to him as well.

Thanks and Regards,
Arun R Murthy
--------------------
Srikanth V, NagaVenkata Sept. 12, 2024, 12:01 p.m. UTC | #5 
Hi Jani,

AUX_RD interval can have maximum value of 256ms. usleep_range() is recommended to be used for 10uS-20ms range as per Documentation/timers/timers-howto.rst



[cid:image001.png@01DB0539.9751B230]



So, switched to fsleep() which takes care of calling respective functions according to the delay.



Regards

Srikanth



-----Original Message-----
From: Murthy, Arun R <arun.r.murthy@intel.com>
Sent: Thursday, September 12, 2024 3:34 PM
To: Jani Nikula <jani.nikula@linux.intel.com>; intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
Cc: Srikanth V, NagaVenkata <nagavenkata.srikanth.v@intel.com>
Subject: RE: [PATCH 1/3] drm/i915/dp: use fsleep instead of usleep_rage for LT



> -----Original Message-----

> From: Jani Nikula <jani.nikula@linux.intel.com<mailto:jani.nikula@linux.intel.com>>

> Sent: Thursday, September 12, 2024 2:32 PM

> To: Murthy, Arun R <arun.r.murthy@intel.com<mailto:arun.r.murthy@intel.com>>;

> intel-xe@lists.freedesktop.org<mailto:intel-xe@lists.freedesktop.org>; intel-gfx@lists.freedesktop.org<mailto:intel-gfx@lists.freedesktop.org>

> Cc: Murthy, Arun R <arun.r.murthy@intel.com<mailto:arun.r.murthy@intel.com>>; Srikanth V, NagaVenkata

> <nagavenkata.srikanth.v@intel.com<mailto:nagavenkata.srikanth.v@intel.com>>

> Subject: Re: [PATCH 1/3] drm/i915/dp: use fsleep instead of

> usleep_rage for LT

>

> On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com<mailto:arun.r.murthy@intel.com>> wrote:

> > Aux RD Interval value depends on the value read from the dpcd

> > register which is updated from the sink device use flseep thereby we

> > adhere to the Documentation/timers/timers-howto.rst

>

> Please explain why instead of just referencing a file.



Sleeping for < 10us use udelay, for 10us to 20ms use usleep_range and for > 10ms use msleep.

fsleep() will call the particular api based on the above condition.



Thanks and Regards,

Arun R Murthy

--------------------
Jani Nikula Sept. 23, 2024, 10:23 a.m. UTC | #6 
On Thu, 12 Sep 2024, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@linux.intel.com>
>> Sent: Thursday, September 12, 2024 2:32 PM
>> To: 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>; Srikanth V, NagaVenkata
>> <nagavenkata.srikanth.v@intel.com>
>> Subject: Re: [PATCH 1/3] drm/i915/dp: use fsleep instead of usleep_rage for LT
>> 
>> On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>> > Aux RD Interval value depends on the value read from the dpcd register
>> > which is updated from the sink device use flseep thereby we adhere to
>> > the Documentation/timers/timers-howto.rst
>> 
>> Please explain why instead of just referencing a file.
>
> Sleeping for < 10us use udelay, for 10us to 20ms use usleep_range and for > 10ms use msleep.
> fsleep() will call the particular api based on the above condition.

Oh, I'm not asking to explain this specifically to me. I'm asking it to
be explained in the commit message, for posterity. A commit message
should always answer the question *why*.

BR,
Jani.
Jani Nikula Sept. 23, 2024, 10:24 a.m. UTC | #7 
On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> Aux RD Interval value depends on the value read from the dpcd register
> which is updated from the sink device use flseep thereby we adhere to
> the Documentation/timers/timers-howto.rst
>
> Signed-off-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>

With the commit message updated to explain why,

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

> ---
>  drivers/gpu/drm/i915/display/intel_dp_link_training.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 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 397cc4ebae52..f41b69840ad9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -898,7 +898,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp,
>  
>  	voltage_tries = 1;
>  	for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
> -		usleep_range(delay_us, 2 * delay_us);
> +		fsleep(delay_us);
>  
>  		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
>  						     link_status) < 0) {
> @@ -1040,7 +1040,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp,
>  	}
>  
>  	for (tries = 0; tries < 5; tries++) {
> -		usleep_range(delay_us, 2 * delay_us);
> +		fsleep(delay_us);
>  
>  		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
>  						     link_status) < 0) {
> @@ -1417,7 +1417,7 @@ intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
>  	deadline = jiffies + msecs_to_jiffies_timeout(400);
>  
>  	for (try = 0; try < max_tries; try++) {
> -		usleep_range(delay_us, 2 * delay_us);
> +		fsleep(delay_us);
>  
>  		/*
>  		 * The delay may get updated. The transmitter shall read the
Jani Nikula Sept. 23, 2024, 10:32 a.m. UTC | #8 
On Thu, 12 Sep 2024, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@linux.intel.com>
>> Sent: Thursday, September 12, 2024 2:36 PM
>> To: 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>; Srikanth V, NagaVenkata
>> <nagavenkata.srikanth.v@intel.com>
>> Subject: Re: [PATCH 1/3] drm/i915/dp: use fsleep instead of usleep_rage for LT
>> 
>> On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>> > Aux RD Interval value depends on the value read from the dpcd register
>> > which is updated from the sink device use flseep thereby we adhere to
>> > the Documentation/timers/timers-howto.rst
>> >
>> > Signed-off-by: Srikanth V NagaVenkata
>> > <nagavenkata.srikanth.v@intel.com>
>> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> 
>> Why do all the patches have two Signed-off-by's?
>> 
> Findings are from Srikanth so giving credits to him as well.

Contrary to popular belief, Signed-off-by: actually has very little to
do with credits and authorship, and everything to do with certifying
that the work can be submitted and merged to the kernel [1].

What you're looking for is Co-developed-by: [2] in combination with
S-o-b.


BR,
Jani.


[1] https://docs.kernel.org/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
[2] https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
Francois Dugast Sept. 23, 2024, 12:59 p.m. UTC | #9 
On Mon, Sep 23, 2024 at 01:24:27PM +0300, Jani Nikula wrote:
> On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> > Aux RD Interval value depends on the value read from the dpcd register
> > which is updated from the sink device use flseep thereby we adhere to
> > the Documentation/timers/timers-howto.rst
> >
> > Signed-off-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> 
> With the commit message updated to explain why,

While at it, please also fix the typo s/usleep_rage/usleep_range/.

Francois

> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_link_training.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 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 397cc4ebae52..f41b69840ad9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -898,7 +898,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp,
> >  
> >  	voltage_tries = 1;
> >  	for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
> > -		usleep_range(delay_us, 2 * delay_us);
> > +		fsleep(delay_us);
> >  
> >  		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
> >  						     link_status) < 0) {
> > @@ -1040,7 +1040,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp,
> >  	}
> >  
> >  	for (tries = 0; tries < 5; tries++) {
> > -		usleep_range(delay_us, 2 * delay_us);
> > +		fsleep(delay_us);
> >  
> >  		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
> >  						     link_status) < 0) {
> > @@ -1417,7 +1417,7 @@ intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
> >  	deadline = jiffies + msecs_to_jiffies_timeout(400);
> >  
> >  	for (try = 0; try < max_tries; try++) {
> > -		usleep_range(delay_us, 2 * delay_us);
> > +		fsleep(delay_us);
> >  
> >  		/*
> >  		 * The delay may get updated. The transmitter shall read the
> 
> -- 
> Jani Nikula, Intel
Jani Nikula Sept. 23, 2024, 1:51 p.m. UTC | #10 
On Mon, 23 Sep 2024, Francois Dugast <francois.dugast@intel.com> wrote:
> On Mon, Sep 23, 2024 at 01:24:27PM +0300, Jani Nikula wrote:
>> On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>> > Aux RD Interval value depends on the value read from the dpcd register
>> > which is updated from the sink device use flseep thereby we adhere to
>> > the Documentation/timers/timers-howto.rst
>> >
>> > Signed-off-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
>> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> 
>> With the commit message updated to explain why,
>
> While at it, please also fix the typo s/usleep_rage/usleep_range/.

usleep_rage does have a certain appeal to it, though... ;)

BR,
Jani.
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 397cc4ebae52..f41b69840ad9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -898,7 +898,7 @@  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp,
 
 	voltage_tries = 1;
 	for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
-		usleep_range(delay_us, 2 * delay_us);
+		fsleep(delay_us);
 
 		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
 						     link_status) < 0) {
@@ -1040,7 +1040,7 @@  intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp,
 	}
 
 	for (tries = 0; tries < 5; tries++) {
-		usleep_range(delay_us, 2 * delay_us);
+		fsleep(delay_us);
 
 		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
 						     link_status) < 0) {
@@ -1417,7 +1417,7 @@  intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
 	deadline = jiffies + msecs_to_jiffies_timeout(400);
 
 	for (try = 0; try < max_tries; try++) {
-		usleep_range(delay_us, 2 * delay_us);
+		fsleep(delay_us);
 
 		/*
 		 * The delay may get updated. The transmitter shall read the