lkml.org 
[lkml]   [2013]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] drm/radeon: rework to new fence interface
Am 19.08.2013 21:37, schrieb Maarten Lankhorst:
> Op 19-08-13 14:35, Christian König schreef:
>> Am 19.08.2013 12:17, schrieb Maarten Lankhorst:
>>> [SNIP]
>>> @@ -190,25 +225,24 @@ void radeon_fence_process(struct radeon_device *rdev, int ring)
>>> }
>>> } while (atomic64_xchg(&rdev->fence_drv[ring].last_seq, seq) > seq);
>>> - if (wake) {
>>> + if (wake)
>>> rdev->fence_drv[ring].last_activity = jiffies;
>>> - wake_up_all(&rdev->fence_queue);
>>> - }
>>> + return wake;
>>> }
>> Very bad idea, when sequence numbers change, you always want to wake up the whole fence queue here.
> Yes, and the callers of this function call wake_up_all or wake_up_all_locked themselves, based on the return value..

And as I said that's a very bad idea. The fence processing shouldn't be
called with any locks held and should be self responsible for activating
any waiters.

>
>>> [SNIP]
>>> +/**
>>> + * radeon_fence_enable_signaling - enable signalling on fence
>>> + * @fence: fence
>>> + *
>>> + * This function is called with fence_queue lock held, and adds a callback
>>> + * to fence_queue that checks if this fence is signaled, and if so it
>>> + * signals the fence and removes itself.
>>> + */
>>> +static bool radeon_fence_enable_signaling(struct fence *f)
>>> +{
>>> + struct radeon_fence *fence = to_radeon_fence(f);
>>> +
>>> + if (atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq) >= fence->seq ||
>>> + !fence->rdev->ddev->irq_enabled)
>>> + return false;
>>> +
>> Do I get that right that you rely on IRQs to be enabled and working here? Cause that would be a quite bad idea from the conceptual side.
> For cross-device synchronization it would be nice to have working irqs, it allows signalling fences faster,
> and it allows for callbacks on completion to be called. For internal usage it's no more required than it was before.

That's a big NAK.

The fence processing is actually very fine tuned to avoid IRQs and as
far as I can see you just leave them enabled by decrementing the atomic
from IRQ context. Additional to that we need allot of special handling
in case of a hardware lockup here, which isn't done if you abuse the
fence interface like this.

Also your approach of leaking the IRQ context outside of the driver is a
very bad idea from the conceptual side. Please don't modify the fence
interface at all and instead use the wait functions already exposed by
radeon_fence.c. If you need some kind of signaling mechanism then wait
inside a workqueue instead.

Christian.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2013-08-20 11:21    [W:0.049 / U:0.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site