> No, you really shouldn't be doing much in the check anyway, it's meant to be
> a lightweight check. If you're not ready yet because of a lockup simply
> return not signaled yet.
It's not only the lockup case from radeon I have in mind here. For
userspace queues it might be necessary to call copy_from_user to figure
out if a fence is signaled or not.
Returning false all the time is probably not a good idea either.
Christian.
Am 22.07.2014 16:05, schrieb Maarten Lankhorst:
> op 22-07-14 14:19, Christian K?nig schreef:
>> Am 22.07.2014 13:57, schrieb Daniel Vetter:
>>> On Tue, Jul 22, 2014 at 01:46:07PM +0200, Daniel Vetter wrote:
>>>> On Tue, Jul 22, 2014 at 10:43:13AM +0200, Christian K?nig wrote:
>>>>> Am 22.07.2014 06:05, schrieb Dave Airlie:
>>>>>> On 9 July 2014 22:29, Maarten Lankhorst <maarten.lankhorst at
>>>>>> canonical.com> wrote:
>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at canonical.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/radeon/radeon.h | 15 +-
>>>>>>> drivers/gpu/drm/radeon/radeon_device.c | 60 ++++++++-
>>>>>>> drivers/gpu/drm/radeon/radeon_fence.c | 223
>>>>>>> ++++++++++++++++++++++++++------
>>>>>>> 3 files changed, 248 insertions(+), 50 deletions(-)
>>>>>>>
>>>>>> From what I can see this is still suffering from the problem that we
>>>>>> need to find a proper solution to,
>>>>>>
>>>>>> My summary of the issues after talking to Jerome and Ben and
>>>>>> re-reading things is:
>>>>>>
>>>>>> We really need to work out a better interface into the drivers to be
>>>>>> able to avoid random atomic entrypoints,
>>>>> Which is exactly what I criticized from the very first beginning. Good to
>>>>> know that I'm not the only one thinking that this isn't such a good idea.
>>>> I guess I've lost context a bit, but which atomic entry point are we
>>>> talking about? Afaics the only one that's mandatory is the is
>>>> fence->signaled callback to check whether a fence really has been
>>>> signalled. It's used internally by the fence code to avoid spurious
>>>> wakeups. Afaik that should be doable already on any hardware. If that's
>>>> not the case then we can always track the signalled state in software and
>>>> double-check in a worker thread before updating the sw state. And wrap
>>>> this all up into a special fence class if there's more than one driver
>>>> needing this.
>>> One thing I've forgotten: The i915 scheduler that's floating around runs
>>> its bottom half from irq context. So I really want to be able to check
>>> fence state from irq context and I also want to make it possible
>>> (possible! not mandatory) to register callbacks which are run from any
>>> context asap after the fence is signalled.
>> NAK, that's just the bad design I've talked about. Checking fence state
>> inside the same driver from interrupt context is OK, because it's the
>> drivers interrupt that we are talking about here.
>>
>> Checking fence status from another drivers interrupt context is what really
>> concerns me here, cause your driver doesn't have the slightest idea if the
>> called driver is really capable of checking the fence right now.
> I think there is a usecase for having atomic context allowed with
> fence_is_signaled, but I don't think there is one for interrupt context, so
> it's good with me if fence_is_signaled cannot be called in interrupt context,
> or with irqs disabled.
>
> fence_enable_sw_signaling disables interrupts because it holds fence->lock,
> so in theory it could be called from any context including interrupts. But no
> sane driver author does that, or at least I hope not..
>
> Would a sanity check like the one below be enough to allay your fears?
> 8<-------
>
> diff --git a/include/linux/fence.h b/include/linux/fence.h
> index d174585b874b..c1a4519ba2f5 100644
> --- a/include/linux/fence.h
> +++ b/include/linux/fence.h
> @@ -143,6 +143,7 @@ struct fence_cb {
> * the second time will be a noop since it was already signaled.
> *
> * Notes on signaled:
> + * Called with interrupts enabled, and never from interrupt context.
> * May set fence->status if returning true.
> *
> * Notes on wait:
> @@ -268,15 +269,29 @@ fence_is_signaled_locked(struct fence *fence)
> static inline bool
> fence_is_signaled(struct fence *fence)
> {
> + bool ret;
> +
> if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> return true;
>
> - if (fence->ops->signaled && fence->ops->signaled(fence)) {
> + if (!fence->ops->signaled)
> + return false;
> +
> + if (config_enabled(CONFIG_PROVE_LOCKING))
> + WARN_ON(in_interrupt() || irqs_disabled());
> +
> + if (config_enabled(CONFIG_DEBUG_ATOMIC_SLEEP))
> + preempt_disable();
> +
> + ret = fence->ops->signaled(fence);
> +
> + if (config_enabled(CONFIG_DEBUG_ATOMIC_SLEEP))
> + preempt_enable();
> +
> + if (ret)
> fence_signal(fence);
> - return true;
> - }
>
> - return false;
> + return ret;
> }
>
> /**
> 8<--------
>
>>> If the radeon hw/driver doesn't want to cope with that complexity we can
>>> fully insolate it with the sw tracked fence state if you don't like
>>> Maarten's radeon implementation. But forcing everyone to forgoe this just
>>> because you don't like it and don't want to use it in radeon doesn't sound
>>> right.
>> While it's clearly a hack Maarten's solution for radeon would indeed work,
>> but that's not really the point here.
>>
>> It's just that I think leaking interrupt context from one driver into
>> another driver is just a really really bad idea from a design point of view.
>>
>> And calling into a driver while in atomic context to check for a fence being
>> signaled doesn't sounds like a good idea either, cause that limits way to
>> much what the called driver can do for checking the status of a fence.
> No, you really shouldn't be doing much in the check anyway, it's meant to be
> a lightweight check. If you're not ready yet because of a lockup simply
> return not signaled yet.
>
> ~Maarten
>
