[Intel-gfx] [RFC 03/12] drm/i915: Support for GuC interrupts
Goel, Akash
akash.goel at intel.com
Sat May 28 09:22:16 UTC 2016
On 5/28/2016 1:26 AM, Chris Wilson wrote:
> On Sat, May 28, 2016 at 01:12:54AM +0530, akash.goel at intel.com wrote:
>> From: Sagar Arun Kamble <sagar.a.kamble at 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.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Signed-off-by: Akash Goel <akash.goel at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>> drivers/gpu/drm/i915/i915_guc_submission.c | 2 +
>> drivers/gpu/drm/i915/i915_irq.c | 100 ++++++++++++++++++++++++++++-
>> drivers/gpu/drm/i915/i915_reg.h | 11 ++++
>> drivers/gpu/drm/i915/intel_drv.h | 3 +
>> drivers/gpu/drm/i915/intel_guc.h | 5 ++
>> drivers/gpu/drm/i915/intel_guc_loader.c | 1 +
>> 7 files changed, 120 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index e4c8e34..7aae033 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1790,6 +1790,7 @@ struct drm_i915_private {
>> u32 gt_irq_mask;
>> u32 pm_irq_mask;
>> u32 pm_rps_events;
>> + u32 guc_events;
>> u32 pipestat_irq_mask[I915_MAX_PIPES];
>>
>> struct i915_hotplug hotplug;
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index da7c242..c2f3a67 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -990,6 +990,8 @@ int intel_guc_suspend(struct drm_device *dev)
>> if (guc->guc_fw.guc_fw_load_status != GUC_FIRMWARE_SUCCESS)
>> return 0;
>>
>> + gen8_disable_guc_interrupts(dev);
>> +
>> ctx = dev_priv->kernel_context;
>>
>> data[0] = HOST2GUC_ACTION_ENTER_S_STATE;
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index caaf1e2..b4294a8 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -170,6 +170,7 @@ static void gen5_assert_iir_is_zero(struct drm_i915_private *dev_priv,
>> } while (0)
>>
>> static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
>> +static void gen8_guc_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
>>
>> /* For display hotplug interrupt */
>> static inline void
>> @@ -400,6 +401,55 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
>> synchronize_irq(dev_priv->dev->irq);
>> }
>>
>> +void gen8_reset_guc_interrupts(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + i915_reg_t reg = gen6_pm_iir(dev_priv);
>
> From the looks of this we have multiple shadows for the same register.
> That's very bad.
>
> Now the platforms might be mutually exclusive, but it is still a mistake
> that will catch us out.
>
Will check how it is in newer platforms.
>> + spin_lock_irq(&dev_priv->irq_lock);
>> + I915_WRITE(reg, dev_priv->guc_events);
>> + I915_WRITE(reg, dev_priv->guc_events);
>
> What? Not even the tiniest of comments to explain?
>
Sorry actually just copied these steps as is from the
gen6_reset_rps_interrupts(), considering that the same set of registers
(IIR, IER, IMR) are involved here.
So the double clearing of IIR followed by posting read could be needed
here also.
>> + POSTING_READ(reg);
>
> Again. Not even the tiniest of comments to explain?
>
>> + spin_unlock_irq(&dev_priv->irq_lock);
>> +}
>> +
>> +void gen8_enable_guc_interrupts(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> + spin_lock_irq(&dev_priv->irq_lock);
>> + if (!dev_priv->guc.interrupts_enabled) {
>> + WARN_ON(I915_READ(gen6_pm_iir(dev_priv)) &
>> + dev_priv->guc_events);
>> + dev_priv->guc.interrupts_enabled = true;
>> + I915_WRITE(gen6_pm_ier(dev_priv),
>> + I915_READ(gen6_pm_ier(dev_priv)) | dev_priv->guc_events);
>
> ier should be known, rmw on the reg should not be required.
>
Sorry same as above, copy paste from gen6_enable_rps_interrupts().
Without rmw, would this be fine ?
if (dev_priv->rps.interrupts_enabled)
I915_WRITE(gen6_pm_ier(dev_priv),
dev_priv->pm_rps_events | dev_priv->guc_events);
else
I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->guc_events);
>> + gen6_enable_pm_irq(dev_priv, dev_priv->guc_events);
>> + }
>> + spin_unlock_irq(&dev_priv->irq_lock);
>> +}
>> +
>> +void gen8_disable_guc_interrupts(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> + spin_lock_irq(&dev_priv->irq_lock);
>> + dev_priv->guc.interrupts_enabled = false;
>> + spin_unlock_irq(&dev_priv->irq_lock);
>> +
>> + cancel_work_sync(&dev_priv->guc.events_work);
>> +
>> + spin_lock_irq(&dev_priv->irq_lock);
>> +
>> + __gen6_disable_pm_irq(dev_priv, dev_priv->guc_events);
>> + I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
>> + ~dev_priv->guc_events);
>> +
>> + spin_unlock_irq(&dev_priv->irq_lock);
>> +
>> + synchronize_irq(dev->irq);
>> +}
>> +
>> /**
>> * bdw_update_port_irq - update DE port interrupt
>> * @dev_priv: driver private
>> @@ -1174,6 +1224,27 @@ out:
>> ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
>> }
>>
>> +static void gen8_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 cancelation during disabling guc interrupts. */
>> + if (!dev_priv->guc.interrupts_enabled) {
>> + spin_unlock_irq(&dev_priv->irq_lock);
>> + return;
>> + }
>> +
>> + DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);
>
> This just shouts that the code is broken.
>
You mean to say that ideally the wakeref_count (& power.usage_count)
should already be non zero here.
>
>> void intel_crt_init(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
>> index 41601c7..e20792d 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -125,6 +125,11 @@ struct intel_guc {
>> struct intel_guc_fw guc_fw;
>> uint32_t log_flags;
>> struct drm_i915_gem_object *log_obj;
>> + /*
>> + * work, interrupts_enabled are protected by dev_priv->irq_lock
>> + */
>> + struct work_struct events_work;
>
> The work gets added here, yet bugs are fixed for the worker in later
> patches. Squash in the bug fixes.
Sorry didn't get this. Are you alluding to cancellation of this
work_item Or flushing of work item from the error handling path ?
Cancellation is being done as a part of disabling interrupt and the call
to disable interrupt is there in intel_guc_suspend(), part of this patch
only.
Best regards
Akash
> -Chris
>
More information about the Intel-gfx
mailing list