Re: [PATCH] radeon/PM Really wait for vblank before reclocking

[Jaime Velasco Juan <jsagarribay@gmail.com>]

El lun. 15 de feb. de 2010, a las 18:52:46 +0100, Rafał Miłecki escribió:
> 2010/2/15 Jaime Velasco Juan <jsagarribay@gmail.com>:
> > The old code used a false condition so it always waited until
> > timeout
> >
> > Signed-off-by: Jaime Velasco Juan <jsagarribay@gmail.com>
> > ---
> >  drivers/gpu/drm/radeon/radeon_pm.c |   10 ++++++----
> >  1 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
> > index a8e151e..842952f 100644
> > --- a/drivers/gpu/drm/radeon/radeon_pm.c
> > +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> > @@ -337,10 +337,12 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
> >                rdev->pm.req_vblank |= (1 << 1);
> >                drm_vblank_get(rdev->ddev, 1);
> >        }
> > -       if (rdev->pm.active_crtcs)
> > -               wait_event_interruptible_timeout(
> > -                       rdev->irq.vblank_queue, 0,
> > -                       msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT));
> > +       if (rdev->pm.active_crtcs) {
> > +               long timeout = msecs_to_jiffies(RADEON_WAIT_VBLANK_TIMEOUT);
> > +               __wait_event_interruptible_timeout(
> > +                       rdev->irq.vblank_queue, 1,
> > +                       timeout);
> > +       }
> 
> Yeah, it seems logic was wrong, thanks.
> 
> Two questions: is there some real reason for using "timeout" variable?
> We don't re-use this anywhere. It seems useless, doesn't make code
> shorter, forces braces usage and you defined this inside braces, not
> on beginning of function, as kernel coding style says. Can you just
> drop this redundant variable?
> 
> Second: could you say what is "better" in using
> __wait_event_interruptible_timeout? Is this faster (doesn't check
> condition) or something?
> 
I just took the code from the definition of wait_event_interruptible_timeout;
that macro checks the condition before calling
__wait_event_interruptible_timeout, where the task really goes to sleep,
but __wait_event_interruptible_timeout changes the timeout parameter, so
it needs to be a variable. I just tried to do the smallest change to the
code. It's true that almost nobody else use this macro directly.

Even with this, I still see some ocasional glitches when reclocking the
engine, but this does not stall rendering for 200ms as before (it was
very noticeable). This has nothing to do with this patch, but is it really
needed to take cp.mutex in radeon_pm_set_clocks? I've been doing some
testing without doing it and it doesn't seem to break anything.

Regards

> -- 
> Rafał

------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel