Re: [PATCH 02/11] drm/i915/guc: Move GuC interrupts related functions from i915_irq.c to intel_guc.c

[Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>]

On 10/18/2017 6:16 PM, Tvrtko Ursulin wrote:
> On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> > GuC interrupts handling is core GuC functionality. Better to keep it
> > with other core functions in intel_guc.c. Since they are used from
> > uC functions, GuC logging, i915 irq handling keeping them grouped in
> > intel_guc.c instead of intel_uc.c.
> >
> > Suggested-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_irq.c      | 70 
> > +----------------------------------
> >   drivers/gpu/drm/i915/intel_drv.h     |  3 --
> >   drivers/gpu/drm/i915/intel_guc.c     | 71 
> > +++++++++++++++++++++++++++++++++++-
> >   drivers/gpu/drm/i915/intel_guc.h     |  4 ++
> >   drivers/gpu/drm/i915/intel_guc_log.c |  6 +--
> >   drivers/gpu/drm/i915/intel_uc.c      |  8 ++--
> >   6 files changed, 81 insertions(+), 81 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index caa6283..84a4bf3 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -203,7 +203,6 @@ static void gen2_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 gen9_guc_irq_handler(struct drm_i915_private *dev_priv, 
> > u32 pm_iir);
> >     /* For display hotplug interrupt */
> >   static inline void
> > @@ -450,38 +449,6 @@ void gen6_disable_rps_interrupts(struct 
> > drm_i915_private *dev_priv)
> >       gen6_reset_rps_interrupts(dev_priv);
> >   }
> >   -void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
> > -{
> > -    spin_lock_irq(&dev_priv->irq_lock);
> > -    gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
> > -    spin_unlock_irq(&dev_priv->irq_lock);
> > -}
> > -
> > -void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
> > -{
> > -    spin_lock_irq(&dev_priv->irq_lock);
> > -    if (!dev_priv->guc.interrupts_enabled) {
> > -        WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> > -                       dev_priv->pm_guc_events);
> > -        dev_priv->guc.interrupts_enabled = true;
> > -        gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> > -    }
> > -    spin_unlock_irq(&dev_priv->irq_lock);
> > -}
> > -
> > -void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
> > -{
> > -    spin_lock_irq(&dev_priv->irq_lock);
> > -    dev_priv->guc.interrupts_enabled = false;
> > -
> > -    gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> > -
> > -    spin_unlock_irq(&dev_priv->irq_lock);
> > -    synchronize_irq(dev_priv->drm.irq);
> > -
> > -    gen9_reset_guc_interrupts(dev_priv);
> > -}
> > -
> >   /**
> >    * bdw_update_port_irq - update DE port interrupt
> >    * @dev_priv: driver private
> > @@ -1474,7 +1441,7 @@ static void gen8_gt_irq_handler(struct 
> > drm_i915_private *dev_priv,
> >           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]);
> > +        intel_guc_irq_handler(dev_priv, gt_iir[2]);
>
> Slight downside that it cannot be inlined any longer, should the 
> compiler decide to do so. But it is probably not a relevant 
> consideration for the frequency of these.
Ok. If needed we can force by making these static inline from header.
Thanks for review.
> >   }
> >     static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
> > @@ -1751,41 +1718,6 @@ 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) {
> > -        /* 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, flush;
> > -
> > -        msg = I915_READ(SOFT_SCRATCH(15));
> > -        flush = msg & (INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED |
> > -                   INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER);
> > -        if (flush) {
> > -            /* Clear the message bits that are handled */
> > -            I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
> > -
> > -            /* Handle flush interrupt in bottom half */
> > -            queue_work(dev_priv->guc.log.runtime.flush_wq,
> > - &dev_priv->guc.log.runtime.flush_work);
> > -
> > -            dev_priv->guc.log.flush_interrupt_count++;
> > -        } else {
> > -            /* Not clearing of unhandled event bits won't result in
> > -             * re-triggering of the interrupt.
> > -             */
> > -        }
> > -    }
> > -}
> > -
> >   static void i9xx_pipestat_irq_reset(struct drm_i915_private *dev_priv)
> >   {
> >       enum pipe pipe;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 792d8ea..39c76ba 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1269,9 +1269,6 @@ void gen8_irq_power_well_post_enable(struct 
> > drm_i915_private *dev_priv,
> >                        u8 pipe_mask);
> >   void gen8_irq_power_well_pre_disable(struct drm_i915_private 
> > *dev_priv,
> >                        u8 pipe_mask);
> > -void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv);
> > -void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv);
> > -void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv);
> >     /* intel_crt.c */
> >   void intel_crt_init(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_guc.c 
> > b/drivers/gpu/drm/i915/intel_guc.c
> > index 10037c0..3a64ae1 100644
> > --- a/drivers/gpu/drm/i915/intel_guc.c
> > +++ b/drivers/gpu/drm/i915/intel_guc.c
> > @@ -274,7 +274,7 @@ int intel_guc_suspend(struct drm_i915_private 
> > *dev_priv)
> >       if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> >           return 0;
> >   -    gen9_disable_guc_interrupts(dev_priv);
> > +    intel_disable_guc_interrupts(dev_priv);
> >         ctx = dev_priv->kernel_context;
> >   @@ -302,7 +302,7 @@ int intel_guc_resume(struct drm_i915_private 
> > *dev_priv)
> >           return 0;
> >         if (i915_modparams.guc_log_level >= 0)
> > -        gen9_enable_guc_interrupts(dev_priv);
> > +        intel_enable_guc_interrupts(dev_priv);
> >         ctx = dev_priv->kernel_context;
> >   @@ -367,3 +367,70 @@ u32 intel_guc_wopcm_size(struct 
> > drm_i915_private *dev_priv)
> >         return wopcm_size;
> >   }
> > +
> > +void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv)
> > +{
> > +    spin_lock_irq(&dev_priv->irq_lock);
> > +    gen6_reset_pm_iir(dev_priv, dev_priv->pm_guc_events);
> > +    spin_unlock_irq(&dev_priv->irq_lock);
> > +}
> > +
> > +void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv)
> > +{
> > +    spin_lock_irq(&dev_priv->irq_lock);
> > +    if (!dev_priv->guc.interrupts_enabled) {
> > +        WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> > +                       dev_priv->pm_guc_events);
> > +        dev_priv->guc.interrupts_enabled = true;
> > +        gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> > +    }
> > +    spin_unlock_irq(&dev_priv->irq_lock);
> > +}
> > +
> > +void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv)
> > +{
> > +    spin_lock_irq(&dev_priv->irq_lock);
> > +    dev_priv->guc.interrupts_enabled = false;
> > +
> > +    gen6_disable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> > +
> > +    spin_unlock_irq(&dev_priv->irq_lock);
> > +    synchronize_irq(dev_priv->drm.irq);
> > +
> > +    intel_reset_guc_interrupts(dev_priv);
> > +}
> > +
> > +void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 
> > gt_iir)
> > +{
> > +    if (gt_iir & GEN9_GUC_TO_HOST_INT_EVENT) {
> > +        /* 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, flush;
> > +
> > +        msg = I915_READ(SOFT_SCRATCH(15));
> > +        flush = msg & (INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED |
> > +                   INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER);
> > +        if (flush) {
> > +            /* Clear the message bits that are handled */
> > +            I915_WRITE(SOFT_SCRATCH(15), msg & ~flush);
> > +
> > +            /* Handle flush interrupt in bottom half */
> > +            queue_work(dev_priv->guc.log.runtime.flush_wq,
> > + &dev_priv->guc.log.runtime.flush_work);
> > +
> > +            dev_priv->guc.log.flush_interrupt_count++;
> > +        } else {
> > +            /* Not clearing of unhandled event bits won't result in
> > +             * re-triggering of the interrupt.
> > +             */
> > +        }
> > +    }
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_guc.h 
> > b/drivers/gpu/drm/i915/intel_guc.h
> > index 418450b..8b26505 100644
> > --- a/drivers/gpu/drm/i915/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/intel_guc.h
> > @@ -116,5 +116,9 @@ static inline u32 guc_ggtt_offset(struct i915_vma 
> > *vma)
> >   int intel_guc_resume(struct drm_i915_private *dev_priv);
> >   struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 
> > size);
> >   u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
> > +void intel_reset_guc_interrupts(struct drm_i915_private *dev_priv);
> > +void intel_enable_guc_interrupts(struct drm_i915_private *dev_priv);
> > +void intel_disable_guc_interrupts(struct drm_i915_private *dev_priv);
> > +void intel_guc_irq_handler(struct drm_i915_private *dev_priv, u32 
> > pm_iir);
> >     #endif
> > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
> > b/drivers/gpu/drm/i915/intel_guc_log.c
> > index 76d3eb1..8120208 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_log.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> > @@ -510,7 +510,7 @@ static void guc_flush_logs(struct intel_guc *guc)
> >           return;
> >         /* First disable the interrupts, will be renabled afterwards */
> > -    gen9_disable_guc_interrupts(dev_priv);
> > +    intel_disable_guc_interrupts(dev_priv);
> >         /* Before initiating the forceful flush, wait for any 
> > pending/ongoing
> >        * flush to complete otherwise forceful flush may not actually 
> > happen.
> > @@ -628,7 +628,7 @@ int i915_guc_log_control(struct drm_i915_private 
> > *dev_priv, u64 control_val)
> >           }
> >             /* GuC logging is currently the only user of Guc2Host 
> > interrupts */
> > -        gen9_enable_guc_interrupts(dev_priv);
> > +        intel_enable_guc_interrupts(dev_priv);
> >       } else {
> >           /* Once logging is disabled, GuC won't generate logs & send an
> >            * interrupt. But there could be some data in the log buffer
> > @@ -662,7 +662,7 @@ void i915_guc_log_unregister(struct 
> > drm_i915_private *dev_priv)
> >         mutex_lock(&dev_priv->drm.struct_mutex);
> >       /* GuC logging is currently the only user of Guc2Host 
> > interrupts */
> > -    gen9_disable_guc_interrupts(dev_priv);
> > +    intel_disable_guc_interrupts(dev_priv);
> >       guc_log_runtime_destroy(&dev_priv->guc);
> >       mutex_unlock(&dev_priv->drm.struct_mutex);
> >   }
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c 
> > b/drivers/gpu/drm/i915/intel_uc.c
> > index 25bd162..b9b9947a0 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -158,7 +158,7 @@ int intel_uc_init_hw(struct drm_i915_private 
> > *dev_priv)
> >           return 0;
> >         guc_disable_communication(guc);
> > -    gen9_reset_guc_interrupts(dev_priv);
> > +    intel_reset_guc_interrupts(dev_priv);
> >         /* We need to notify the guc whenever we change the GGTT */
> >       i915_ggtt_enable_guc(dev_priv);
> > @@ -215,7 +215,7 @@ int intel_uc_init_hw(struct drm_i915_private 
> > *dev_priv)
> >       intel_huc_auth(&dev_priv->huc);
> >       if (i915_modparams.enable_guc_submission) {
> >           if (i915_modparams.guc_log_level >= 0)
> > -            gen9_enable_guc_interrupts(dev_priv);
> > +            intel_enable_guc_interrupts(dev_priv);
> >             ret = i915_guc_submission_enable(dev_priv);
> >           if (ret)
> > @@ -241,7 +241,7 @@ int intel_uc_init_hw(struct drm_i915_private 
> > *dev_priv)
> >        */
> >   err_interrupts:
> >       guc_disable_communication(guc);
> > -    gen9_disable_guc_interrupts(dev_priv);
> > +    intel_disable_guc_interrupts(dev_priv);
> >   err_log_capture:
> >       guc_capture_load_err_log(guc);
> >   err_submission:
> > @@ -282,7 +282,7 @@ void intel_uc_fini_hw(struct drm_i915_private 
> > *dev_priv)
> >       guc_disable_communication(&dev_priv->guc);
> >         if (i915_modparams.enable_guc_submission) {
> > -        gen9_disable_guc_interrupts(dev_priv);
> > +        intel_disable_guc_interrupts(dev_priv);
> >           i915_guc_submission_fini(dev_priv);
> >       }
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> Regards,
>
> Tvrtko

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