Re: [RFC PATCH] drm/radeon: rework to new fence interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux