On 7/11/2016 7:13 PM, Tvrtko Ursulin wrote:
On 11/07/16 14:38, Goel, Akash wrote:
On 7/11/2016 6:53 PM, Tvrtko Ursulin wrote:
On 11/07/16 14:15, Goel, Akash wrote:
On 7/11/2016 4:00 PM, Tvrtko Ursulin wrote:
+static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv,
u32 gt_iir)
+{
+ if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
+ spin_lock(&dev_priv->irq_lock);
+ if (dev_priv->guc.interrupts_enabled) {
+ /* Sample the log buffer flush related bits & clear them
+ * out now itself from the message identity register to
+ * minimize the probability of losing a flush interrupt,
+ * when there are back to back flush interrupts.
+ * There can be a new flush interrupt, for different log
+ * buffer type (like for ISR), whilst Host is handling
+ * one (for DPC). Since same bit is used in message
+ * register for ISR & DPC, it could happen that GuC
+ * sets the bit for 2nd interrupt but Host clears out
+ * the bit on handling the 1st interrupt.
+ */
+ u32 msg = I915_READ(SOFT_SCRATCH(15)) &
+ (GUC2HOST_MSG_CRASH_DUMP_POSTED |
+ GUC2HOST_MSG_FLUSH_LOG_BUFFER);
+ if (msg) {
+ /* Clear the message bits that are handled */
+ I915_WRITE(SOFT_SCRATCH(15),
+ I915_READ(SOFT_SCRATCH(15)) & ~msg);
+
+ /* Handle flush interrupt event in bottom half */
+ queue_work(dev_priv->wq,
&dev_priv->guc.events_work);
Since the later patch is changing this to use a thread, since you have
established worker is too slow - especially the shared one - I would
really recommend you start with the kthread straight away. Not have
the
worker for a while in the same series and then later change it to a
thread.
Actually it won't be appropriate to say that shared worker thread is
too
slow, but having a dedicated kthread definitely helps.
I kept the kthread patch at the last so that as per the response,
review comments can drop it also.
I think it should only be one implementation in the patch series. If we
agreed on a kthread make it so from the start.
Agree but actually right now, added the kthread patch more as a RFC and
presumed this won't be the final version of the series.
Will do the needful, as per the review comments, in the next version.
Ack.
And describe in the commit message why it was selected etc.
+ }
+ }
+ spin_unlock(&dev_priv->irq_lock);
Why does the above needs to be done under the irq_lock ?
Using the irq_lock for 'guc.interrupts_enabled', especially useful
while disabling the interrupt.
Why? I don't see how it gains you anything and so it seems preferable
not to hold it over mmio accesses.
Yes not needed for the mmio access part.
Just needed for the inspection of 'guc.interrupts_enabled' value.
Will reorder the code.
You don't need it just for reading that value, you can just drop it.
Its not strictly needed as its a mere read. But as per my limited
understanding, without the spinlock (which provides an implicit barrier
also) ISR might miss the reset of 'interrupts_enabled' flag, from a
thread on other CPU, and queue the new work. The update will be
visible eventually though. And same applies to the case when
'interrupts_enabled' flag is set from other CPU.
Good practice to use locks for accessing shared variables ?.
Best regards
Akash
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
