Re: [PATCH 05/17] drm/i915: Support for GuC interrupts

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 11/07/16 14:15, Goel, Akash wrote:
> On 7/11/2016 4:00 PM, Tvrtko Ursulin wrote:
>>
>> On 10/07/16 14:41, akash.goel@intel.com wrote:
>>> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>
>>> There are certain types of interrupts which Host can recieve from GuC.
>>> GuC ukernel sends an interrupt to Host for certain events, like for
>>> example retrieve/consume the logs generated by ukernel.
>>> This patch adds support to receive interrupts from GuC but currently
>>> enables & partially handles only the interrupt sent by GuC ukernel.
>>> Future patches will add support for handling other interrupt types.
>>>
>>> v2:
>>> - Use common low level routines for PM IER/IIR programming (Chris)
>>> - Rename interrupt functions to gen9_xxx from gen8_xxx (Chris)
>>> - Replace disabling of wake ref asserts with rpm get/put (Chris)
>>>
>>> v3:
>>> - Update comments for more clarity. (Tvrtko)
>>> - Remove the masking of GuC interrupt, which was kept masked till the
>>>    start of bottom half, its not really needed as there is only a
>>>    single instance of work item & wq is ordered. (Tvrtko)
>>>
>>> v4:
>>> - Rebase.
>>> - Rename guc_events to pm_guc_events so as to be indicative of the
>>>    register/control block it is associated with. (Chris)
>>> - Add handling for back to back log buffer flush interrupts.
>>>
>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h            |  1 +
>>>   drivers/gpu/drm/i915/i915_guc_submission.c |  5 ++
>>>   drivers/gpu/drm/i915/i915_irq.c            | 98
>>> ++++++++++++++++++++++++++++--
>>>   drivers/gpu/drm/i915/i915_reg.h            | 11 ++++
>>>   drivers/gpu/drm/i915/intel_drv.h           |  3 +
>>>   drivers/gpu/drm/i915/intel_guc.h           |  4 ++
>>>   drivers/gpu/drm/i915/intel_guc_loader.c    |  4 ++
>>>   7 files changed, 122 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index c3a579f..6e2ddfa 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1794,6 +1794,7 @@ struct drm_i915_private {
>>>       u32 pm_imr;
>>>       u32 pm_ier;
>>>       u32 pm_rps_events;
>>> +    u32 pm_guc_events;
>>>       u32 pipestat_irq_mask[I915_MAX_PIPES];
>>>
>>>       struct i915_hotplug hotplug;
>>>
>>> +
>>>   /**
>>>    * bdw_update_port_irq - update DE port interrupt
>>>    * @dev_priv: driver private
>>> @@ -1174,6 +1208,21 @@ static void gen6_pm_rps_work(struct work_struct
>>> *work)
>>>       mutex_unlock(&dev_priv->rps.hw_lock);
>>>   }
>>>
>>> +static void gen9_guc2host_events_work(struct work_struct *work)
>>> +{
>>> +    struct drm_i915_private *dev_priv =
>>> +        container_of(work, struct drm_i915_private, guc.events_work);
>>> +
>>> +    spin_lock_irq(&dev_priv->irq_lock);
>>> +    /* Speed up work cancellation during disabling guc interrupts. */
>>> +    if (!dev_priv->guc.interrupts_enabled) {
>>> +        spin_unlock_irq(&dev_priv->irq_lock);
>>> +        return;
>>> +    }
>>> +    spin_unlock_irq(&dev_priv->irq_lock);
>>> +
>>> +    /* TODO: Handle the events for which GuC interrupted host */
>>> +}
>>>
>>>   /**
>>>    * ivybridge_parity_work - Workqueue called when a parity error
>>> interrupt
>>> @@ -1346,11 +1395,13 @@ static irqreturn_t gen8_gt_irq_ack(struct
>>> drm_i915_private *dev_priv,
>>>               DRM_ERROR("The master control interrupt lied (GT3)!\n");
>>>       }
>>>
>>> -    if (master_ctl & GEN8_GT_PM_IRQ) {
>>> +    if (master_ctl & (GEN8_GT_PM_IRQ | GEN8_GT_GUC_IRQ)) {
>>>           gt_iir[2] = I915_READ_FW(GEN8_GT_IIR(2));
>>> -        if (gt_iir[2] & dev_priv->pm_rps_events) {
>>> +        if (gt_iir[2] & (dev_priv->pm_rps_events |
>>> +                 dev_priv->pm_guc_events)) {
>>>               I915_WRITE_FW(GEN8_GT_IIR(2),
>>> -                      gt_iir[2] & dev_priv->pm_rps_events);
>>> +                      gt_iir[2] & (dev_priv->pm_rps_events |
>>> +                           dev_priv->pm_guc_events));
>>>               ret = IRQ_HANDLED;
>>>           } else
>>>               DRM_ERROR("The master control interrupt lied (PM)!\n");
>>> @@ -1382,6 +1433,9 @@ static void gen8_gt_irq_handler(struct
>>> drm_i915_private *dev_priv,
>>>
>>>       if (gt_iir[2] & dev_priv->pm_rps_events)
>>>           gen6_rps_irq_handler(dev_priv, gt_iir[2]);
>>> +
>>> +    if (gt_iir[2] & dev_priv->pm_guc_events)
>>> +        gen9_guc_irq_handler(dev_priv, gt_iir[2]);
>>>   }
>>>
>>>   static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
>>> @@ -1628,6 +1682,38 @@ static void gen6_rps_irq_handler(struct
>>> drm_i915_private *dev_priv, u32 pm_iir)
>>>       }
>>>   }
>>>
>>> +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.

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.

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx