Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over readx_poll_timeout()

Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over readx_poll_timeout()

[Ville Syrjälä]

On Fri, Jun 27, 2025 at 02:36:32PM +0300, Jani Nikula wrote:
> Unify on using read_poll_timeout() throughout instead of mixing with
> readx_poll_timeout(). While the latter can be ever so slightly simpler,
> they are both complicated enough that it's better to unify on one
> approach only.
> 
> While at it, better separate the handling of error returns from
> drm_dp_dpcd_readb() and the actual status byte. This is best achieved by
> inlining the read_fec_detected_status() function.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 33 +++++++++---------------
>  1 file changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 0405396c7750..fc4587311607 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -2339,34 +2339,25 @@ static void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>  		drm_dbg_kms(display->drm, "Failed to clear FEC detected flags\n");
>  }
>  
> -static int read_fec_detected_status(struct drm_dp_aux *aux)
> -{
> -	int ret;
> -	u8 status;
> -
> -	ret = drm_dp_dpcd_readb(aux, DP_FEC_STATUS, &status);
> -	if (ret < 0)
> -		return ret;
> -
> -	return status;
> -}
> -
>  static int wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled)
>  {
>  	struct intel_display *display = to_intel_display(aux->drm_dev);
>  	int mask = enabled ? DP_FEC_DECODE_EN_DETECTED : DP_FEC_DECODE_DIS_DETECTED;
> -	int status;
> -	int err;
> +	u8 status = 0;
> +	int ret, err;
>  
> -	err = readx_poll_timeout(read_fec_detected_status, aux, status,
> -				 status & mask || status < 0,
> -				 10000, 200000);
> +	ret = read_poll_timeout(drm_dp_dpcd_readb, err,
> +				err || (status & mask),
> +				10 * 1000, 200 * 1000, false,
> +				aux, DP_FEC_STATUS, &status);

I think I hate these macros. It's very hard to tell from this
soup what is actually being done here.

The 'val', 'op', and 'args' look very disconnected here even though
they are always part of the same thing. Is there a reason they can't
just be a single 'op' parameter like we have in wait_for() so you can
actually see the code?

Ie.
read_poll_timeout(err = drm_dp_dpcd_readb(aux, DP_FEC_STATUS, &status),
		  err || (status & mask),
                  10 * 1000, 200 * 1000, false);
?

>  
> -	if (err || status < 0) {
> +	/* Either can be non-zero, but not both */
> +	ret = ret ?: err;
> +	if (ret) {
>  		drm_dbg_kms(display->drm,
> -			    "Failed waiting for FEC %s to get detected: %d (status %d)\n",
> -			    str_enabled_disabled(enabled), err, status);
> -		return err ? err : status;
> +			    "Failed waiting for FEC %s to get detected: %d (status 0x%02x)\n",
> +			    str_enabled_disabled(enabled), ret, status);
> +		return ret;
>  	}
>  
>  	return 0;
> -- 
> 2.39.5

-- 
Ville Syrjälä
Intel

Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over readx_poll_timeout()

[Jani Nikula]

On Fri, 27 Jun 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Jun 27, 2025 at 02:36:32PM +0300, Jani Nikula wrote:
>> Unify on using read_poll_timeout() throughout instead of mixing with
>> readx_poll_timeout(). While the latter can be ever so slightly simpler,
>> they are both complicated enough that it's better to unify on one
>> approach only.
>> 
>> While at it, better separate the handling of error returns from
>> drm_dp_dpcd_readb() and the actual status byte. This is best achieved by
>> inlining the read_fec_detected_status() function.
>> 
>> Cc: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_ddi.c | 33 +++++++++---------------
>>  1 file changed, 12 insertions(+), 21 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>> index 0405396c7750..fc4587311607 100644
>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> @@ -2339,34 +2339,25 @@ static void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>>  		drm_dbg_kms(display->drm, "Failed to clear FEC detected flags\n");
>>  }
>>  
>> -static int read_fec_detected_status(struct drm_dp_aux *aux)
>> -{
>> -	int ret;
>> -	u8 status;
>> -
>> -	ret = drm_dp_dpcd_readb(aux, DP_FEC_STATUS, &status);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	return status;
>> -}
>> -
>>  static int wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled)
>>  {
>>  	struct intel_display *display = to_intel_display(aux->drm_dev);
>>  	int mask = enabled ? DP_FEC_DECODE_EN_DETECTED : DP_FEC_DECODE_DIS_DETECTED;
>> -	int status;
>> -	int err;
>> +	u8 status = 0;
>> +	int ret, err;
>>  
>> -	err = readx_poll_timeout(read_fec_detected_status, aux, status,
>> -				 status & mask || status < 0,
>> -				 10000, 200000);
>> +	ret = read_poll_timeout(drm_dp_dpcd_readb, err,
>> +				err || (status & mask),
>> +				10 * 1000, 200 * 1000, false,
>> +				aux, DP_FEC_STATUS, &status);
>
> I think I hate these macros. It's very hard to tell from this
> soup what is actually being done here.

The thing is, I hate __wait_for(), wait_for(), wait_for_us(),
wait_for_atomic_us(), and wait_for_atomic() even more.

It's also very hard to figure out what is actually going on with
them. The timeouts are arbitrarily either ms or us. wait_for_us() is
atomic depending on the timeout. __wait_for() Wmax parameter actually
isn't the max sleep, it's 2*Wmax-2. Some of them have exponentially
growing sleeps, while some arbitrarily don't.

It's a fscking mess, and people randomly choose whichever version with
no idea what's actually going on behind the scenes.

> The 'val', 'op', and 'args' look very disconnected here even though
> they are always part of the same thing. Is there a reason they can't
> just be a single 'op' parameter like we have in wait_for() so you can
> actually see the code?
>
> Ie.
> read_poll_timeout(err = drm_dp_dpcd_readb(aux, DP_FEC_STATUS, &status),
> 		  err || (status & mask),
>                   10 * 1000, 200 * 1000, false);
> ?

Internally the macro has:

#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
				sleep_before_read, args...) \

...

		(val) = op(args); \

So you do need to provide an lvalue val, and you need to be able to add
() after op. I think GCC allows not passing varargs. IOW you'd need to
implement another macro (which could be used to implement the existing
one, but not the other way round).

I'm really not enthusiastic about blocking this series waiting on that
kind of refactoring in iopoll.h which might happen, or might not,
considering there's no active maintainer for iopoll.h.

So yeah, the interface isn't great, and I'm not claiming it is, but it
is *one* *single* *documented* *interface* that's used across the
kernel. On the whole, warts and all, I think it's still much better than
what we currently have. And it breaks the dependency on i915_utils.h.

I've carefully tried to do the line breaks so that it's always:

        read_poll_timeout(op, val,
                          cond,
                          sleep_us, timeout_us, sleep_before_read,
                          args...);

I think that helps a bit.


BR,
Jani.


>
>>  
>> -	if (err || status < 0) {
>> +	/* Either can be non-zero, but not both */
>> +	ret = ret ?: err;
>> +	if (ret) {
>>  		drm_dbg_kms(display->drm,
>> -			    "Failed waiting for FEC %s to get detected: %d (status %d)\n",
>> -			    str_enabled_disabled(enabled), err, status);
>> -		return err ? err : status;
>> +			    "Failed waiting for FEC %s to get detected: %d (status 0x%02x)\n",
>> +			    str_enabled_disabled(enabled), ret, status);
>> +		return ret;
>>  	}
>>  
>>  	return 0;
>> -- 
>> 2.39.5

-- 
Jani Nikula, Intel

Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over readx_poll_timeout()

[Ville Syrjälä]

On Fri, Jun 27, 2025 at 04:34:23PM +0300, Jani Nikula wrote:
> On Fri, 27 Jun 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Jun 27, 2025 at 02:36:32PM +0300, Jani Nikula wrote:
> >> Unify on using read_poll_timeout() throughout instead of mixing with
> >> readx_poll_timeout(). While the latter can be ever so slightly simpler,
> >> they are both complicated enough that it's better to unify on one
> >> approach only.
> >> 
> >> While at it, better separate the handling of error returns from
> >> drm_dp_dpcd_readb() and the actual status byte. This is best achieved by
> >> inlining the read_fec_detected_status() function.
> >> 
> >> Cc: Imre Deak <imre.deak@intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_ddi.c | 33 +++++++++---------------
> >>  1 file changed, 12 insertions(+), 21 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> index 0405396c7750..fc4587311607 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> @@ -2339,34 +2339,25 @@ static void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> >>  		drm_dbg_kms(display->drm, "Failed to clear FEC detected flags\n");
> >>  }
> >>  
> >> -static int read_fec_detected_status(struct drm_dp_aux *aux)
> >> -{
> >> -	int ret;
> >> -	u8 status;
> >> -
> >> -	ret = drm_dp_dpcd_readb(aux, DP_FEC_STATUS, &status);
> >> -	if (ret < 0)
> >> -		return ret;
> >> -
> >> -	return status;
> >> -}
> >> -
> >>  static int wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled)
> >>  {
> >>  	struct intel_display *display = to_intel_display(aux->drm_dev);
> >>  	int mask = enabled ? DP_FEC_DECODE_EN_DETECTED : DP_FEC_DECODE_DIS_DETECTED;
> >> -	int status;
> >> -	int err;
> >> +	u8 status = 0;
> >> +	int ret, err;
> >>  
> >> -	err = readx_poll_timeout(read_fec_detected_status, aux, status,
> >> -				 status & mask || status < 0,
> >> -				 10000, 200000);
> >> +	ret = read_poll_timeout(drm_dp_dpcd_readb, err,
> >> +				err || (status & mask),
> >> +				10 * 1000, 200 * 1000, false,
> >> +				aux, DP_FEC_STATUS, &status);
> >
> > I think I hate these macros. It's very hard to tell from this
> > soup what is actually being done here.
> 
> The thing is, I hate __wait_for(), wait_for(), wait_for_us(),
> wait_for_atomic_us(), and wait_for_atomic() even more.
> 
> It's also very hard to figure out what is actually going on with
> them. The timeouts are arbitrarily either ms or us. wait_for_us() is
> atomic depending on the timeout. __wait_for() Wmax parameter actually
> isn't the max sleep, it's 2*Wmax-2. Some of them have exponentially
> growing sleeps, while some arbitrarily don't.
> 
> It's a fscking mess, and people randomly choose whichever version with
> no idea what's actually going on behind the scenes.
> 
> > The 'val', 'op', and 'args' look very disconnected here even though
> > they are always part of the same thing. Is there a reason they can't
> > just be a single 'op' parameter like we have in wait_for() so you can
> > actually see the code?
> >
> > Ie.
> > read_poll_timeout(err = drm_dp_dpcd_readb(aux, DP_FEC_STATUS, &status),
> > 		  err || (status & mask),
> >                   10 * 1000, 200 * 1000, false);
> > ?
> 
> Internally the macro has:
> 
> #define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
> 				sleep_before_read, args...) \
> 
> ...
> 
> 		(val) = op(args); \
> 
> So you do need to provide an lvalue val, and you need to be able to add
> () after op. I think GCC allows not passing varargs. IOW you'd need to
> implement another macro (which could be used to implement the existing
> one, but not the other way round).

Just get rid of the 'args' and 'val' and it'll work just fine.
Then it'll be almost identical to wait_for() (basically just missing the
increasing backoff stuff).

AFAICS this thing was originally added just for reading a single
register and checking some bit/etc, so the name made some sense.
But now we're abusing it for all kinds of random things, so even
the name no longer makes that much sense.

So I think just something like this would work fine for us:

diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index 91324c331a4b..9c38fd732028 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -14,27 +14,24 @@
 #include <linux/io.h>
 
 /**
- * read_poll_timeout - Periodically poll an address until a condition is
- *			met or a timeout occurs
- * @op: accessor function (takes @args as its arguments)
- * @val: Variable to read the value into
- * @cond: Break condition (usually involving @val)
- * @sleep_us: Maximum time to sleep between reads in us (0 tight-loops). Please
- *            read usleep_range() function description for details and
+ * poll_timeout - Periodically poll and perform an operaion until
+ *                a condition is met or a timeout occurs
+ *
+ * @op: Operation
+ * @cond: Break condition
+ * @sleep_us: Maximum time to sleep between operations in us (0 tight-loops).
+ *            Please read usleep_range() function description for details and
  *            limitations.
  * @timeout_us: Timeout in us, 0 means never timeout
- * @sleep_before_read: if it is true, sleep @sleep_us before read.
- * @args: arguments for @op poll
+ * @sleep_before_read: if it is true, sleep @sleep_us before operation.
  *
  * When available, you'll probably want to use one of the specialized
  * macros defined below rather than this macro directly.
  *
- * Returns: 0 on success and -ETIMEDOUT upon a timeout. In either
- * case, the last read value at @args is stored in @val. Must not
+ * Returns: 0 on success and -ETIMEDOUT upon a timeout. Must not
  * be called from atomic context if sleep_us or timeout_us are used.
  */
-#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
-				sleep_before_read, args...) \
+#define poll_timeout(op, cond, sleep_us, timeout_us, sleep_before_read) \
 ({ \
 	u64 __timeout_us = (timeout_us); \
 	unsigned long __sleep_us = (sleep_us); \
@@ -43,12 +40,12 @@
 	if (sleep_before_read && __sleep_us) \
 		usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
 	for (;;) { \
-		(val) = op(args); \
+		op; \
 		if (cond) \
 			break; \
 		if (__timeout_us && \
 		    ktime_compare(ktime_get(), __timeout) > 0) { \
-			(val) = op(args); \
+			op; \
 			break; \
 		} \
 		if (__sleep_us) \
@@ -58,6 +55,30 @@
 	(cond) ? 0 : -ETIMEDOUT; \
 })
 
+/**
+ * read_poll_timeout - Periodically poll an address until a condition is
+ *                     met or a timeout occurs
+ * @op: accessor function (takes @args as its arguments)
+ * @val: Variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @sleep_us: Maximum time to sleep between reads in us (0 tight-loops). Please
+ *            read usleep_range() function description for details and
+ *            limitations.
+ * @timeout_us: Timeout in us, 0 means never timeout
+ * @sleep_before_read: if it is true, sleep @sleep_us before read.
+ * @args: arguments for @op poll
+ *
+ * When available, you'll probably want to use one of the specialized
+ * macros defined below rather than this macro directly.
+ *
+ * Returns: 0 on success and -ETIMEDOUT upon a timeout. In either
+ * case, the last read value at @args is stored in @val. Must not
+ * be called from atomic context if sleep_us or timeout_us are used.
+ */
+#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
+			  sleep_before_read, args...) \
+	poll_timeout((val) = op(args), cond, sleep_us, timeout_us, sleep_before_read)
+
 /**
  * read_poll_timeout_atomic - Periodically poll an address until a condition is
  * 				met or a timeout occurs
-- 
2.49.0

-- 
Ville Syrjälä
Intel

Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over readx_poll_timeout()

[Jani Nikula]

On Fri, 27 Jun 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Jun 27, 2025 at 04:34:23PM +0300, Jani Nikula wrote:
>> Internally the macro has:
>> 
>> #define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
>> 				sleep_before_read, args...) \
>> 
>> ...
>> 
>> 		(val) = op(args); \
>> 
>> So you do need to provide an lvalue val, and you need to be able to add
>> () after op. I think GCC allows not passing varargs. IOW you'd need to
>> implement another macro (which could be used to implement the existing
>> one, but not the other way round).
>
> Just get rid of the 'args' and 'val' and it'll work just fine.
> Then it'll be almost identical to wait_for() (basically just missing the
> increasing backoff stuff).
>
> AFAICS this thing was originally added just for reading a single
> register and checking some bit/etc, so the name made some sense.
> But now we're abusing it for all kinds of random things, so even
> the name no longer makes that much sense.

Yeah, evolution not intelligent design.

> So I think just something like this would work fine for us:

LGTM, with the _atomic version for completeness.

Want to send it to lkml?


BR,
Jani.


>
> diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> index 91324c331a4b..9c38fd732028 100644
> --- a/include/linux/iopoll.h
> +++ b/include/linux/iopoll.h
> @@ -14,27 +14,24 @@
>  #include <linux/io.h>
>  
>  /**
> - * read_poll_timeout - Periodically poll an address until a condition is
> - *			met or a timeout occurs
> - * @op: accessor function (takes @args as its arguments)
> - * @val: Variable to read the value into
> - * @cond: Break condition (usually involving @val)
> - * @sleep_us: Maximum time to sleep between reads in us (0 tight-loops). Please
> - *            read usleep_range() function description for details and
> + * poll_timeout - Periodically poll and perform an operaion until
> + *                a condition is met or a timeout occurs
> + *
> + * @op: Operation
> + * @cond: Break condition
> + * @sleep_us: Maximum time to sleep between operations in us (0 tight-loops).
> + *            Please read usleep_range() function description for details and
>   *            limitations.
>   * @timeout_us: Timeout in us, 0 means never timeout
> - * @sleep_before_read: if it is true, sleep @sleep_us before read.
> - * @args: arguments for @op poll
> + * @sleep_before_read: if it is true, sleep @sleep_us before operation.
>   *
>   * When available, you'll probably want to use one of the specialized
>   * macros defined below rather than this macro directly.
>   *
> - * Returns: 0 on success and -ETIMEDOUT upon a timeout. In either
> - * case, the last read value at @args is stored in @val. Must not
> + * Returns: 0 on success and -ETIMEDOUT upon a timeout. Must not
>   * be called from atomic context if sleep_us or timeout_us are used.
>   */
> -#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
> -				sleep_before_read, args...) \
> +#define poll_timeout(op, cond, sleep_us, timeout_us, sleep_before_read) \
>  ({ \
>  	u64 __timeout_us = (timeout_us); \
>  	unsigned long __sleep_us = (sleep_us); \
> @@ -43,12 +40,12 @@
>  	if (sleep_before_read && __sleep_us) \
>  		usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
>  	for (;;) { \
> -		(val) = op(args); \
> +		op; \
>  		if (cond) \
>  			break; \
>  		if (__timeout_us && \
>  		    ktime_compare(ktime_get(), __timeout) > 0) { \
> -			(val) = op(args); \
> +			op; \
>  			break; \
>  		} \
>  		if (__sleep_us) \
> @@ -58,6 +55,30 @@
>  	(cond) ? 0 : -ETIMEDOUT; \
>  })
>  
> +/**
> + * read_poll_timeout - Periodically poll an address until a condition is
> + *                     met or a timeout occurs
> + * @op: accessor function (takes @args as its arguments)
> + * @val: Variable to read the value into
> + * @cond: Break condition (usually involving @val)
> + * @sleep_us: Maximum time to sleep between reads in us (0 tight-loops). Please
> + *            read usleep_range() function description for details and
> + *            limitations.
> + * @timeout_us: Timeout in us, 0 means never timeout
> + * @sleep_before_read: if it is true, sleep @sleep_us before read.
> + * @args: arguments for @op poll
> + *
> + * When available, you'll probably want to use one of the specialized
> + * macros defined below rather than this macro directly.
> + *
> + * Returns: 0 on success and -ETIMEDOUT upon a timeout. In either
> + * case, the last read value at @args is stored in @val. Must not
> + * be called from atomic context if sleep_us or timeout_us are used.
> + */
> +#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
> +			  sleep_before_read, args...) \
> +	poll_timeout((val) = op(args), cond, sleep_us, timeout_us, sleep_before_read)
> +
>  /**
>   * read_poll_timeout_atomic - Periodically poll an address until a condition is
>   * 				met or a timeout occurs
> -- 
> 2.49.0

-- 
Jani Nikula, Intel

Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over readx_poll_timeout()

[Ville Syrjälä]

On Fri, Jun 27, 2025 at 07:26:22PM +0300, Jani Nikula wrote:
> On Fri, 27 Jun 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Jun 27, 2025 at 04:34:23PM +0300, Jani Nikula wrote:
> >> Internally the macro has:
> >> 
> >> #define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
> >> 				sleep_before_read, args...) \
> >> 
> >> ...
> >> 
> >> 		(val) = op(args); \
> >> 
> >> So you do need to provide an lvalue val, and you need to be able to add
> >> () after op. I think GCC allows not passing varargs. IOW you'd need to
> >> implement another macro (which could be used to implement the existing
> >> one, but not the other way round).
> >
> > Just get rid of the 'args' and 'val' and it'll work just fine.
> > Then it'll be almost identical to wait_for() (basically just missing the
> > increasing backoff stuff).
> >
> > AFAICS this thing was originally added just for reading a single
> > register and checking some bit/etc, so the name made some sense.
> > But now we're abusing it for all kinds of random things, so even
> > the name no longer makes that much sense.
> 
> Yeah, evolution not intelligent design.
> 
> > So I think just something like this would work fine for us:
> 
> LGTM, with the _atomic version for completeness.

The other differences between wait_for() and read_poll_timeout()
I see are:

- read_poll_timeout() always evaluates 'cond' at least twice.
  For some things I think it would make sense to omit 'op'
  entirely so we don't have to introduce pointless variables
  in the caller (eg. poll_timeout(, pipe_scanline_is_moving(...), ...))

  but the double evaluation of 'cond' there is not desirable.
  Should be an easy change to make read_poll_timeout() more
  like wait_for() and stash the return value into a variable.

- ktime_get() vs. ktime_get_raw(). I suppose it doesn't really
  matter too much which is used?

- 'op' and 'cond' are evaluated twice during the same iteration of
  the loop for the timeout case in read_poll_timeout(). wait_for()
  is a bit more optimal here by sampling the timeout first, then
  doing the 'op'+'cond', and finally checking whether the timeout
  happened.

  I suppose optimizing the timeout case isn't very critical. Though
  the code would be a bit less repetitive, with the caveat that we
  need an extra variable for the timeout result.

- wait_for() has an explicit compiler barrier to make sure 'cond'
  and the timeout evaluation aren't reordered. Though I think it's
  in the wrong spot for the cases where 'op' is the one that samples
  the thing that 'cond' checks.

  However I *think* ktime_get() being a function call should be enough
  to prevent that reordering from happening?

I guess I'll see what I can cook up to make this stuff
more agreeable...

-- 
Ville Syrjälä
Intel

Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over readx_poll_timeout()

[David Laight]

On Fri, 27 Jun 2025 19:26:22 +0300
Jani Nikula <jani.nikula@intel.com> wrote:

> On Fri, 27 Jun 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Jun 27, 2025 at 04:34:23PM +0300, Jani Nikula wrote:  
> >> Internally the macro has:
> >> 
> >> #define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
> >> 				sleep_before_read, args...) \
> >> 
> >> ...
> >> 
> >> 		(val) = op(args); \
> >> 
> >> So you do need to provide an lvalue val, and you need to be able to add
> >> () after op. I think GCC allows not passing varargs. IOW you'd need to
> >> implement another macro (which could be used to implement the existing
> >> one, but not the other way round).  
> >
> > Just get rid of the 'args' and 'val' and it'll work just fine.
> > Then it'll be almost identical to wait_for() (basically just missing the
> > increasing backoff stuff).
> >
> > AFAICS this thing was originally added just for reading a single
> > register and checking some bit/etc, so the name made some sense.
> > But now we're abusing it for all kinds of random things, so even
> > the name no longer makes that much sense.  
> 
> Yeah, evolution not intelligent design.
> 
> > So I think just something like this would work fine for us:  
> 
> LGTM, with the _atomic version for completeness.
> 
> Want to send it to lkml?
> 
> 
> BR,
> Jani.
> 
> 
> >
> > diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> > index 91324c331a4b..9c38fd732028 100644
> > --- a/include/linux/iopoll.h
> > +++ b/include/linux/iopoll.h
> > @@ -14,27 +14,24 @@
> >  #include <linux/io.h>
> >  
> >  /**
> > - * read_poll_timeout - Periodically poll an address until a condition is
> > - *			met or a timeout occurs
> > - * @op: accessor function (takes @args as its arguments)
> > - * @val: Variable to read the value into
> > - * @cond: Break condition (usually involving @val)
> > - * @sleep_us: Maximum time to sleep between reads in us (0 tight-loops). Please
> > - *            read usleep_range() function description for details and
> > + * poll_timeout - Periodically poll and perform an operaion until
> > + *                a condition is met or a timeout occurs
> > + *
> > + * @op: Operation
> > + * @cond: Break condition
> > + * @sleep_us: Maximum time to sleep between operations in us (0 tight-loops).
> > + *            Please read usleep_range() function description for details and
> >   *            limitations.
> >   * @timeout_us: Timeout in us, 0 means never timeout
> > - * @sleep_before_read: if it is true, sleep @sleep_us before read.
> > - * @args: arguments for @op poll
> > + * @sleep_before_read: if it is true, sleep @sleep_us before operation.
> >   *
> >   * When available, you'll probably want to use one of the specialized
> >   * macros defined below rather than this macro directly.
> >   *
> > - * Returns: 0 on success and -ETIMEDOUT upon a timeout. In either
> > - * case, the last read value at @args is stored in @val. Must not
> > + * Returns: 0 on success and -ETIMEDOUT upon a timeout. Must not
> >   * be called from atomic context if sleep_us or timeout_us are used.
> >   */
> > -#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
> > -				sleep_before_read, args...) \
> > +#define poll_timeout(op, cond, sleep_us, timeout_us, sleep_before_read) \

I might name it poll_timeout_us(...) so that the units are obvious
at the call site.
There are so many different units for timeouts its is worth always
appending _sec, _ms, _us (etc) just to avoid all the silly bugs.

	David