[Intel-gfx] [PATCH 05/17] drm/i915: Support for GuC interrupts

[Goel, Akash]

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.

> 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.

Best regards
Akash

> Regards,
>
> Tvrtko
>
>