Nouveau - Jul 2014 - [PATCH 09/17] drm/radeon: use common fence implementation for fences

Am 22.07.2014 16:27, schrieb Maarten Lankhorst:
> op 22-07-14 16:24, Christian K?nig schreef:
>>> 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.
> Having userspace implement a fence sounds like an awful idea, why would you
want to do that?
Marketing moves in mysterious ways. Don't ask me, but that the direction 
it currently moves with userspace queues and IOMMU etc...
> A fence could be exported to userspace, but that would only mean it can
wait for it to be signaled with an interface like poll..
Yeah agree totally, but the point for the fence interface is that I 
can't predict what's necessary to check if a fence is signaled or not on
future hardware.

For the currently available radeon hardware I can say that reading a 
value from a kernel page is pretty much all you need. But for older 
hardware that was reading from a register which might become very tricky 
if the hardware is power off or currently inside a reset cycle.

Because off this I would avoid any such interface if it's not absolutely 
required by some use case, and currently I don't see this requirement 
because the functionality you want to archive could be implemented 
without this.

Christian.
>
> ~Maarten
>

op 22-07-14 16:39, Christian K?nig schreef:
> Am 22.07.2014 16:27, schrieb Maarten Lankhorst:
>> op 22-07-14 16:24, Christian K?nig schreef:
>>>> 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.
>> Having userspace implement a fence sounds like an awful idea, why would
you want to do that?
>
> Marketing moves in mysterious ways. Don't ask me, but that the
direction it currently moves with userspace queues and IOMMU etc...
>
>> A fence could be exported to userspace, but that would only mean it can
wait for it to be signaled with an interface like poll..
>
> Yeah agree totally, but the point for the fence interface is that I
can't predict what's necessary to check if a fence is signaled or not on
future hardware.
>
> For the currently available radeon hardware I can say that reading a value
from a kernel page is pretty much all you need. But for older hardware that was
reading from a register which might become very tricky if the hardware is power
off or currently inside a reset cycle.
>
> Because off this I would avoid any such interface if it's not
absolutely required by some use case, and currently I don't see this
requirement because the functionality you want to archive could be implemented
without this.
Oh? I've already done that in radeon_fence, there is no way enable_signaling
will fiddle with hardware registers during a reset cycle.
I've also made sure that __radeon_fence_is_signaled grabs exclusive_lock in
read mode before touching any hw state.

Older hardware also doesn't implement optimus, so I think power off is not
much of a worry for them, if you could point me at the checking done for that I
could make sure that this is the case.

~Maarten

Am 22.07.2014 16:47, schrieb Maarten Lankhorst:
> op 22-07-14 16:39, Christian K?nig schreef:
>> Am 22.07.2014 16:27, schrieb Maarten Lankhorst:
>>> op 22-07-14 16:24, Christian K?nig schreef:
>>>>> 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.
>>> Having userspace implement a fence sounds like an awful idea, why
would you want to do that?
>> Marketing moves in mysterious ways. Don't ask me, but that the
direction it currently moves with userspace queues and IOMMU etc...
>>
>>> A fence could be exported to userspace, but that would only mean it
can wait for it to be signaled with an interface like poll..
>> Yeah agree totally, but the point for the fence interface is that I
can't predict what's necessary to check if a fence is signaled or not on
future hardware.
>>
>> For the currently available radeon hardware I can say that reading a
value from a kernel page is pretty much all you need. But for older hardware
that was reading from a register which might become very tricky if the hardware
is power off or currently inside a reset cycle.
>>
>> Because off this I would avoid any such interface if it's not
absolutely required by some use case, and currently I don't see this
requirement because the functionality you want to archive could be implemented
without this.
> Oh? I've already done that in radeon_fence, there is no way
enable_signaling will fiddle with hardware registers during a reset cycle.
> I've also made sure that __radeon_fence_is_signaled grabs
exclusive_lock in read mode before touching any hw state.
>
> Older hardware also doesn't implement optimus, so I think power off is
not much of a worry for them, if you could point me at the checking done for
that I could make sure that this is the case.
I'm not talking about any specific radeon hardware or use case here. As 
far as I can see you indeed solved all driver problems with the current 
interface design.

The question I'm raising is if the current interface design needs as 
complex as it is. And my answer to this is a clear *no*, so why do you 
want to stick with this design? I still haven't understood that.

If it's just to support a further feature of direct synchronization in 
interrupt context between different drivers then I must clearly say that 
this is a NAK, cause you add complexity to the kernel that isn't necessary.

Christian.
>
> ~Maarten
>

On Tue, Jul 22, 2014 at 4:39 PM, Christian K?nig
<christian.koenig at amd.com> wrote:
> Am 22.07.2014 16:27, schrieb Maarten Lankhorst:
>
>> op 22-07-14 16:24, Christian K?nig schreef:
>>>>
>>>> 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.
>>
>> Having userspace implement a fence sounds like an awful idea, why would
>> you want to do that?
>
>
> Marketing moves in mysterious ways. Don't ask me, but that the
direction it
> currently moves with userspace queues and IOMMU etc...
Fence-based syncing between userspace queues submitted stuff through
doorbells and anything submitted by the general simply wont work.
Which is why I think the doorbell is a stupid interface since I just
don't see cameras and v4l devices implementing all that complexity to
get a pure userspace side sync solution.

But that's a different problem really, and I guess marketing will
eventually figure this one out, too.
-Daniel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

On Tue, Jul 22, 2014 at 11:19 AM, Daniel Vetter <daniel at ffwll.ch>
wrote:
> On Tue, Jul 22, 2014 at 4:39 PM, Christian K?nig
> <christian.koenig at amd.com> wrote:
>> Am 22.07.2014 16:27, schrieb Maarten Lankhorst:
>>
>>> op 22-07-14 16:24, Christian K?nig schreef:
>>>>>
>>>>> 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.
>>>
>>> Having userspace implement a fence sounds like an awful idea, why
would
>>> you want to do that?
>>
>>
>> Marketing moves in mysterious ways. Don't ask me, but that the
direction it
>> currently moves with userspace queues and IOMMU etc...
>
> Fence-based syncing between userspace queues submitted stuff through
> doorbells and anything submitted by the general simply wont work.
> Which is why I think the doorbell is a stupid interface since I just
> don't see cameras and v4l devices implementing all that complexity to
> get a pure userspace side sync solution.
>
Like it or not this is what a lot of application writers want (look at
mantle and metal and similar new APIs or android synpts).  Having
queues and fences in userspace allows the application to structure
things to best fit their own task graphs.  The app can decide how to
deal with dependencies and synchronization explicitly instead of
blocking the queues in the kernel for everyone.  Anyway, this is
getting off topic.

Alex