Re: [PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics
On Fre, 2012-06-01 at 12:44 +0200, Christian König wrote: > On 01.06.2012 08:30, Michel Dänzer wrote: > > On Fre, 2012-06-01 at 08:19 +0200, Michel Dänzer wrote: > >> I think this might introduce a race condition: > >> > >> Thread 0 Thread 1 > >> > >> atomic_inc_return() returns 1 > >> spin_lock_irqsave() > >> atomic_dec_and_test() > >> radeon_irq_set() > >> > >> => the interrupt won't be enabled. > > Hrmm, I messed up the formatting there, let me try one more time: > > > > Thread 0Thread 1 > > > > atomic_inc_return() returns 1 > > spin_lock_irqsave() > > atomic_dec_and_test() > > radeon_irq_set() > > > > > Nope that isn't a problem, cause what you really get in your example is: > > Thread 0 Thread 1 > > atomic_inc_return() returns 1 > spin_lock_irqsave() > atomic_dec_and_test() > radeon_irq_set() > spin_unlock_irqrestore() > spin_lock_irqsave() > radeon_irq_set() > spin_unlock_irqrestore() > > > So testing the atomic counters just determines if we need an update of > the irq registers or not, and since a significant change will always > trigger an update we can make sure that the irq regs are always set to > the last known state. We might call radeon_irq_set more often than > necessary, but that won't hurt us and is really unlikely. Yeah, I also realized in the meantime that the race can't happen. I blame it on lack of caffeine. :) > Also I have found the real reason why using the atomic for preventing ih > recursion didn't worked as expected - it was just a stupid typo in my > patch. But thanks for the comment anyway, it got me to look into the > right direction for the bug. Glad to hear that! -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Debian, X and DRI developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics
On 01.06.2012 08:30, Michel Dänzer wrote: On Fre, 2012-06-01 at 08:19 +0200, Michel Dänzer wrote: I think this might introduce a race condition: Thread 0 Thread 1 atomic_inc_return() returns 1 spin_lock_irqsave() atomic_dec_and_test() radeon_irq_set() => the interrupt won't be enabled. Hrmm, I messed up the formatting there, let me try one more time: Thread 0Thread 1 atomic_inc_return() returns 1 spin_lock_irqsave() atomic_dec_and_test() radeon_irq_set() Nope that isn't a problem, cause what you really get in your example is: Thread 0Thread 1 atomic_inc_return() returns 1 spin_lock_irqsave() atomic_dec_and_test() radeon_irq_set() spin_unlock_irqrestore() spin_lock_irqsave() radeon_irq_set() spin_unlock_irqrestore() So testing the atomic counters just determines if we need an update of the irq registers or not, and since a significant change will always trigger an update we can make sure that the irq regs are always set to the last known state. We might call radeon_irq_set more often than necessary, but that won't hurt us and is really unlikely. Also I have found the real reason why using the atomic for preventing ih recursion didn't worked as expected - it was just a stupid typo in my patch. But thanks for the comment anyway, it got me to look into the right direction for the bug. Cheers, Christian. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics
On Fre, 2012-06-01 at 08:19 +0200, Michel Dänzer wrote: > > I think this might introduce a race condition: > > Thread 0 Thread 1 > > atomic_inc_return() returns 1 > spin_lock_irqsave() > atomic_dec_and_test() > radeon_irq_set() > > => the interrupt won't be enabled. Hrmm, I messed up the formatting there, let me try one more time: Thread 0Thread 1 atomic_inc_return() returns 1 spin_lock_irqsave() atomic_dec_and_test() radeon_irq_set() -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Debian, X and DRI developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics
On Don, 2012-05-31 at 22:16 +0200, Christian König wrote:
>
> So we can skip the looking.
'locking'
> diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c
> b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> index 73cd0fd..52f85ba 100644
> --- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> @@ -87,16 +87,7 @@ void radeon_driver_irq_preinstall_kms(struct drm_device
> *dev)
>
> int radeon_driver_irq_postinstall_kms(struct drm_device *dev)
> {
> - struct radeon_device *rdev = dev->dev_private;
> - unsigned long irqflags;
> - unsigned i;
> -
> dev->max_vblank_count = 0x001f;
> - spin_lock_irqsave(&rdev->irq.lock, irqflags);
> - for (i = 0; i < RADEON_NUM_RINGS; i++)
> - rdev->irq.sw_int[i] = true;
> - radeon_irq_set(rdev);
> - spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> return 0;
> }
Why does this function no longer need to enable SW interrupts? If it
really doesn't, that might be material for a separate patch.
> @@ -225,25 +216,28 @@ void radeon_irq_kms_sw_irq_get(struct radeon_device
> *rdev, int ring)
> {
> unsigned long irqflags;
>
> - spin_lock_irqsave(&rdev->irq.lock, irqflags);
> - if (rdev->ddev->irq_enabled && (++rdev->irq.sw_refcount[ring] == 1)) {
> - rdev->irq.sw_int[ring] = true;
> + if (!rdev->ddev->irq_enabled)
> + return;
> +
> + if (atomic_inc_return(&rdev->irq.ring_int[ring]) == 1) {
> + spin_lock_irqsave(&rdev->irq.lock, irqflags);
> radeon_irq_set(rdev);
> + spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> }
> - spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> }
>
> void radeon_irq_kms_sw_irq_put(struct radeon_device *rdev, int ring)
> {
> unsigned long irqflags;
>
> - spin_lock_irqsave(&rdev->irq.lock, irqflags);
> - BUG_ON(rdev->ddev->irq_enabled && rdev->irq.sw_refcount[ring] <= 0);
> - if (rdev->ddev->irq_enabled && (--rdev->irq.sw_refcount[ring] == 0)) {
> - rdev->irq.sw_int[ring] = false;
> + if (!rdev->ddev->irq_enabled)
> + return;
> +
> + if (atomic_dec_and_test(&rdev->irq.ring_int[ring])) {
> + spin_lock_irqsave(&rdev->irq.lock, irqflags);
> radeon_irq_set(rdev);
> + spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> }
> - spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> }
>
> void radeon_irq_kms_pflip_irq_get(struct radeon_device *rdev, int crtc)
I think this might introduce a race condition:
Thread 0 Thread 1
atomic_inc_return() returns 1
spin_lock_irqsave()
atomic_dec_and_test()
radeon_irq_set()
=> the interrupt won't be enabled.
Maybe this explains why you couldn't remove the spinlock in patch 6?
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Debian, X and DRI developer
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics
> -Original Message- > From: Sylvain BERTRAND [mailto:sylw...@legeek.net] > Sent: Thursday, May 24, 2012 1:59 PM > To: Christian König > Cc: j.gli...@gmail.com; Koenig, Christian; dri-devel@lists.freedesktop.org > Subject: Re: [PATCH 08/10] drm/radeon: replace pflip and sw_int counters > with atomics > > > + atomic_tring_int[RADEON_NUM_RINGS]; > > bool > crtc_vblank_int[RADEON_MAX_CRTCS]; > > - boolpflip[RADEON_MAX_CRTCS]; > > - int > pflip_refcount[RADEON_MAX_CRTCS]; > > + atomic_tpflip[RADEON_MAX_CRTCS]; > > Hi, > > Does the linux API mandates atomic_t to be a 32bits word? AFAIK it is, at least for the platforms we care about. But since this depends on the vertical refresh frequency even a 8bit counter should do fine. On the other hand it was an accident that those patches hit the maillinglist in the first place, cause only the first four where supposed to be send out (my fault, sorry). This one isn't tested beside compiling, and I don't think it will work out of the box. Cheers, Christian. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics
> Does the linux API mandates atomic_t to be a 32bits word? AFAIK it is, at least for the platforms we care about. ... >>> >>> Then, the proper course of action would be to add to the linux API, sized >>> atomic operation first, wouldn't it? >> >> No, atomic is fine for this, I think only sparc32 had 24-bit atomics, >> and if you can get sparc32 + a radeon, >> then you can keep both halves. > > And even that is a lie now :-) > > http://lwn.net/Articles/71732/ Ok then: atomic_t means exactly 32 bits! -- Sylvain ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics
On Thu, May 24, 2012 at 1:53 PM, Dave Airlie wrote: > On Thu, May 24, 2012 at 1:46 PM, Sylvain BERTRAND wrote: Does the linux API mandates atomic_t to be a 32bits word? >>> >>> AFAIK it is, at least for the platforms we care about. >>> ... >> >> Then, the proper course of action would be to add to the linux API, sized >> atomic operation first, wouldn't it? >> > > No, atomic is fine for this, I think only sparc32 had 24-bit atomics, > and if you can get sparc32 + a radeon, > then you can keep both halves. And even that is a lie now :-) http://lwn.net/Articles/71732/ Dave. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics
On Thu, May 24, 2012 at 1:46 PM, Sylvain BERTRAND wrote: >>> Does the linux API mandates atomic_t to be a 32bits word? >> >> AFAIK it is, at least for the platforms we care about. >> ... > > Then, the proper course of action would be to add to the linux API, sized > atomic operation first, wouldn't it? > No, atomic is fine for this, I think only sparc32 had 24-bit atomics, and if you can get sparc32 + a radeon, then you can keep both halves. Dave. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics
>> Does the linux API mandates atomic_t to be a 32bits word? > > AFAIK it is, at least for the platforms we care about. > ... Then, the proper course of action would be to add to the linux API, sized atomic operation first, wouldn't it? -- Sylvain ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics
> + atomic_tring_int[RADEON_NUM_RINGS]; > boolcrtc_vblank_int[RADEON_MAX_CRTCS]; > - boolpflip[RADEON_MAX_CRTCS]; > - int pflip_refcount[RADEON_MAX_CRTCS]; > + atomic_tpflip[RADEON_MAX_CRTCS]; Hi, Does the linux API mandates atomic_t to be a 32bits word? Regards, -- Sylvain ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
