drm/vmwgfx: Replace schedule() with schedule_hrtimeout() in fallback wait

[PATCH] drm/vmwgfx: Replace schedule() with schedule_hrtimeout() in fallback wait

Posted by Pavan Bobba 4 months, 1 week ago

Replace the open-coded polling with schedule() in vmw_fallback_wait()
by schedule_hrtimeout(). The old code wakes up at jiffy granularity and
leads to unnecessary CPU wakeups during fence waits.

schedule_hrtimeout() provides high-resolution sleep with finer control,
reducing CPU utilization without affecting fence correctness. For the
non-interruptible case, use schedule_timeout_uninterruptible().

Signed-off-by: Pavan Bobba <opensource206@gmail.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_irq.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c b/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
index 05773eb394d3..64045b0efafc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
@@ -202,16 +202,12 @@ int vmw_fallback_wait(struct vmw_private *dev_priv,
 		if (lazy)
 			schedule_timeout(1);
 		else if ((++count & 0x0F) == 0) {
-			/**
-			 * FIXME: Use schedule_hr_timeout here for
-			 * newer kernels and lower CPU utilization.
-			 */
-
-			__set_current_state(TASK_RUNNING);
-			schedule();
-			__set_current_state((interruptible) ?
-					    TASK_INTERRUPTIBLE :
-					    TASK_UNINTERRUPTIBLE);
+			ktime_t delta = ktime_set(0, NSEC_PER_MSEC);
+
+			if (interruptible)
+				schedule_hrtimeout(&delta, HRTIMER_MODE_REL);
+			else
+				schedule_timeout_uninterruptible(delta);
 		}
 		if (interruptible && signal_pending(current)) {
 			ret = -ERESTARTSYS;
-- 
2.43.0

Re: [PATCH] drm/vmwgfx: Replace schedule() with schedule_hrtimeout() in fallback wait

Posted by Zack Rusin 3 months, 2 weeks ago

On Sun, Sep 28, 2025 at 1:49 AM Pavan Bobba <opensource206@gmail.com> wrote:
>
> Replace the open-coded polling with schedule() in vmw_fallback_wait()
> by schedule_hrtimeout(). The old code wakes up at jiffy granularity and
> leads to unnecessary CPU wakeups during fence waits.
>
> schedule_hrtimeout() provides high-resolution sleep with finer control,
> reducing CPU utilization without affecting fence correctness. For the
> non-interruptible case, use schedule_timeout_uninterruptible().
>
> Signed-off-by: Pavan Bobba <opensource206@gmail.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_irq.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c b/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
> index 05773eb394d3..64045b0efafc 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
> @@ -202,16 +202,12 @@ int vmw_fallback_wait(struct vmw_private *dev_priv,
>                 if (lazy)
>                         schedule_timeout(1);
>                 else if ((++count & 0x0F) == 0) {
> -                       /**
> -                        * FIXME: Use schedule_hr_timeout here for
> -                        * newer kernels and lower CPU utilization.
> -                        */
> -
> -                       __set_current_state(TASK_RUNNING);
> -                       schedule();
> -                       __set_current_state((interruptible) ?
> -                                           TASK_INTERRUPTIBLE :
> -                                           TASK_UNINTERRUPTIBLE);
> +                       ktime_t delta = ktime_set(0, NSEC_PER_MSEC);
> +
> +                       if (interruptible)
> +                               schedule_hrtimeout(&delta, HRTIMER_MODE_REL);
> +                       else
> +                               schedule_timeout_uninterruptible(delta);
>                 }
>                 if (interruptible && signal_pending(current)) {
>                         ret = -ERESTARTSYS;
> --

I don't remember exactly the schedule family of functions but isn't
schedule_hrtimeout leaving the task in a running state? In general it
looks like with the patch the task's current state doesn't match what
was expected, plus I'm not sure if I quite get why the uninterruptible
non-lazy case is being replaced with a lazy wait of NSEC_PER_MSEC's.
It'd be great if you could explain a little bit better what you're
doing here because the commit message is missing an explanation for
either of those.

z

Re: [PATCH] drm/vmwgfx: Replace schedule() with schedule_hrtimeout() in fallback wait

Posted by opensource india 3 months, 1 week ago

Hi Zack Rusin,

On Mon, Oct 20, 2025 at 9:48 AM Zack Rusin <zack.rusin@broadcom.com> wrote:
>

> I don't remember exactly the schedule family of functions but isn't
> schedule_hrtimeout leaving the task in a running state? In general it
> looks like with the patch the task's current state doesn't match what
> was expected, plus I'm not sure if I quite get why the uninterruptible
> non-lazy case is being replaced with a lazy wait of NSEC_PER_MSEC's.
> It'd be great if you could explain a little bit better what you're
> doing here because the commit message is missing an explanation for
> either of those.
>
> z

Thank you for checking the patch.

The existing code does not specify any fixed wait time during the
fence wait. It simply invokes schedule(),
which means the task can be rescheduled immediately to check the fence
status again.

By using the high-resolution timer family of functions, we can specify
an explicit sleep duration.
In this patch, the sleep time is set to 1 ms, ensuring that the fence
status is checked at fixed 1 ms intervals.

This approach allows the CPU to be released to other tasks for a
deterministic period,
thereby reducing unnecessary CPU wakeups while maintaining timely
fence checks(FIXME expected the same).

Re: [PATCH] drm/vmwgfx: Replace schedule() with schedule_hrtimeout() in fallback wait

Posted by Zack Rusin 3 months, 1 week ago

On Tue, Oct 28, 2025 at 7:06 AM opensource india
<opensource206@gmail.com> wrote:
>
> Hi Zack Rusin,
>
> On Mon, Oct 20, 2025 at 9:48 AM Zack Rusin <zack.rusin@broadcom.com> wrote:
> >
>
> > I don't remember exactly the schedule family of functions but isn't
> > schedule_hrtimeout leaving the task in a running state? In general it
> > looks like with the patch the task's current state doesn't match what
> > was expected, plus I'm not sure if I quite get why the uninterruptible
> > non-lazy case is being replaced with a lazy wait of NSEC_PER_MSEC's.
> > It'd be great if you could explain a little bit better what you're
> > doing here because the commit message is missing an explanation for
> > either of those.
> >
> > z
>
> Thank you for checking the patch.
>
> The existing code does not specify any fixed wait time during the
> fence wait. It simply invokes schedule(),
> which means the task can be rescheduled immediately to check the fence
> status again.
>
> By using the high-resolution timer family of functions, we can specify
> an explicit sleep duration.
> In this patch, the sleep time is set to 1 ms, ensuring that the fence
> status is checked at fixed 1 ms intervals.
>
> This approach allows the CPU to be released to other tasks for a
> deterministic period,
> thereby reducing unnecessary CPU wakeups while maintaining timely
> fence checks(FIXME expected the same).

Sorry, but that doesn't answer any of my questions. I can see what the
patch is doing, but I'd love to know why. Same with the wait period:
why have you picked 1ms? To me that seems like introducing a huge
latency into fence waits, so I'd expect to see numbers that back it
up. What benchmarks have you run that show the CPU utilization and
FPS/score both before and after this patch that would justify that
wait period?

z

Re: [PATCH] drm/vmwgfx: Replace schedule() with schedule_hrtimeout() in fallback wait

Posted by opensource india 4 months ago

On Sun, Sep 28, 2025 at 11:19 AM Pavan Bobba <opensource206@gmail.com> wrote:
>
> Replace the open-coded polling with schedule() in vmw_fallback_wait()
> by schedule_hrtimeout(). The old code wakes up at jiffy granularity and
> leads to unnecessary CPU wakeups during fence waits.
>
> schedule_hrtimeout() provides high-resolution sleep with finer control,
> reducing CPU utilization without affecting fence correctness. For the
> non-interruptible case, use schedule_timeout_uninterruptible().
>
> Signed-off-by: Pavan Bobba <opensource206@gmail.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_irq.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c b/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
> index 05773eb394d3..64045b0efafc 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
> @@ -202,16 +202,12 @@ int vmw_fallback_wait(struct vmw_private *dev_priv,
>                 if (lazy)
>                         schedule_timeout(1);
>                 else if ((++count & 0x0F) == 0) {
> -                       /**
> -                        * FIXME: Use schedule_hr_timeout here for
> -                        * newer kernels and lower CPU utilization.
> -                        */
> -
> -                       __set_current_state(TASK_RUNNING);
> -                       schedule();
> -                       __set_current_state((interruptible) ?
> -                                           TASK_INTERRUPTIBLE :
> -                                           TASK_UNINTERRUPTIBLE);
> +                       ktime_t delta = ktime_set(0, NSEC_PER_MSEC);
> +
> +                       if (interruptible)
> +                               schedule_hrtimeout(&delta, HRTIMER_MODE_REL);
> +                       else
> +                               schedule_timeout_uninterruptible(delta);
>                 }
>                 if (interruptible && signal_pending(current)) {
>                         ret = -ERESTARTSYS;
> --
> 2.43.0
>

anyone please review this patch

Re: [PATCH] drm/vmwgfx: Replace schedule() with schedule_hrtimeout() in fallback wait

Posted by opensource india 3 months, 3 weeks ago

On Tue, Oct 7, 2025 at 4:21 PM opensource india <opensource206@gmail.com> wrote:
>
> On Sun, Sep 28, 2025 at 11:19 AM Pavan Bobba <opensource206@gmail.com> wrote:
> >
> > Replace the open-coded polling with schedule() in vmw_fallback_wait()
> > by schedule_hrtimeout(). The old code wakes up at jiffy granularity and
> > leads to unnecessary CPU wakeups during fence waits.
> >
> > schedule_hrtimeout() provides high-resolution sleep with finer control,
> > reducing CPU utilization without affecting fence correctness. For the
> > non-interruptible case, use schedule_timeout_uninterruptible().
> >
> > Signed-off-by: Pavan Bobba <opensource206@gmail.com>
> > ---
> >  drivers/gpu/drm/vmwgfx/vmwgfx_irq.c | 16 ++++++----------
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c b/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
> > index 05773eb394d3..64045b0efafc 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_irq.c
> > @@ -202,16 +202,12 @@ int vmw_fallback_wait(struct vmw_private *dev_priv,
> >                 if (lazy)
> >                         schedule_timeout(1);
> >                 else if ((++count & 0x0F) == 0) {
> > -                       /**
> > -                        * FIXME: Use schedule_hr_timeout here for
> > -                        * newer kernels and lower CPU utilization.
> > -                        */
> > -
> > -                       __set_current_state(TASK_RUNNING);
> > -                       schedule();
> > -                       __set_current_state((interruptible) ?
> > -                                           TASK_INTERRUPTIBLE :
> > -                                           TASK_UNINTERRUPTIBLE);
> > +                       ktime_t delta = ktime_set(0, NSEC_PER_MSEC);
> > +
> > +                       if (interruptible)
> > +                               schedule_hrtimeout(&delta, HRTIMER_MODE_REL);
> > +                       else
> > +                               schedule_timeout_uninterruptible(delta);
> >                 }
> >                 if (interruptible && signal_pending(current)) {
> >                         ret = -ERESTARTSYS;
> > --
> > 2.43.0
> >
>
> anyone please review this patch?

Hi all, can anyone please review this?