Re: [PATCH v2] drm/i915/bxt: Broxton decoupled MMIO

[Carlos Santa <carlos.santa@intel.com>]

On Mon, 2016-09-26 at 21:23 +0100, Tvrtko Ursulin wrote:
> On 26/09/2016 12:08, Paneri, Praveen wrote:
> > 
> > 
> > On 9/23/2016 3:19 PM, Tvrtko Ursulin wrote:
> > > 
> > > 
> > > Hi,
> > > 
> > > On 19/09/2016 18:15, Praveen Paneri wrote:
> > > 
> [snip]
> 
> > 
> > > 
> > > 
> > > > 
> > > > +
> > > >   enum forcewake_domains
> > > >   intel_uncore_forcewake_for_reg(struct drm_i915_private
> > > > *dev_priv,
> > > >                      i915_reg_t reg, unsigned int op);
> > > > @@ -2854,6 +2864,7 @@ struct drm_i915_cmd_table {
> > > >   #define GT_FREQUENCY_MULTIPLIER 50
> > > >   #define GEN9_FREQ_SCALER 3
> > > >   +#define HAS_DECOUPLED_MMIO(dev_priv) (IS_BROXTON(dev_priv)
> > > > &&
> > > > IS_BXT_REVID(dev_priv, BXT_REVID_C0, REVID_FOREVER))
> > > There is a recent patch series from Carlos Santa which moved all
> > > these
> > > type of things to device info. So I think you have to make this
> > > compliant with that new style.
> > I looked into it. Could you suggest where can I add the check for
> > BXT 
> > C0 revision?
> > Will this be okay
> > #define HAS_DECOUPLED_MMIO(dev) (INTEL_INFO(dev)-
> > >has_decoupled_mmio 
> > && IS_BXT_REVID(to_i915(dev), BXT_REVID_C0, REVID_FOREVER))
> Good point. I suggest a quick chat with Carlos then to see what was
> the 
> plan for situation like this one.
> 
> [snip]

This looks good to me, the main point was to do some clean-up for the
legacy features already there but also to move to a single approach
(device info) when adding new features. I don't see any other way to
specifically check for that version of BXT and this MMIO feature, so
that looks good too.

Carlos


> 
> > 
> > > 
> > > 
> > > > 
> > > > +
> > > > +    ctrl_reg_data |= reg;
> > > > +    ctrl_reg_data |= (operation << GEN9_DECOUPLED_OP_SHIFT);
> > > > +    ctrl_reg_data |= (pd_engine << GEN9_DECOUPLED_PD_SHIFT);
> > > > +    __raw_i915_write32(dev_priv, GEN9_DECOUPLED_REG0_DW1,
> > > > ctrl_reg_data);
> > > > +
> > > > +    ctrl_reg_data |= GEN9_DECOUPLED_DW1_GO;
> > > > +    __raw_i915_write32(dev_priv, GEN9_DECOUPLED_REG0_DW1,
> > > > ctrl_reg_data);
> > > > +
> > > > +    if (wait_for_atomic((__raw_i915_read32(dev_priv,
> > > > +            GEN9_DECOUPLED_REG0_DW1) & GEN9_DECOUPLED_DW1_GO)
> > > > == 0,
> > > > +            FORCEWAKE_ACK_TIMEOUT_MS))
> > > > +        DRM_ERROR("Decoupled MMIO wait timed out\n");
> > > > +
> > > Is FORCEWAKE_ACK_TIMEOUT_MS correct/relevant for decoupled MMIO?
> > > It is
> > > potentially quite a long atomic wait.
> > This is max wait time. We might not wait for that long but I can
> > still 
> > check on it.
> Cool, I do think we need to know and document this in the commit
> and/or 
> code comments where a brief explanation of decoupled mmio will be.
> 
> > 
> > > 
> > > 
> > > Would it be worth returning some fixed value in the case of a
> > > timeout?
> > > Might be better than random stuff? (applies in the 64-bit read
> > > case)
> > This is same as forcewake implementation. If we change it, what
> > would 
> > be the appropriate fixed value to be returned?
> Another good point. In that case I suppose it doesn't matter so can 
> leave it like it was. It can only theoretically affect 64-bit reads,
> yes?
> 
> > 
> > > 
> > > 
> > > > 
> > > > +    if (operation == GEN9_DECOUPLED_OP_READ)
> > > > +        *ptr_data = __raw_i915_read32(dev_priv,
> > > > +                GEN9_DECOUPLED_REG0_DW0);
> > > > +}
> > > > +
> > > >   #define GEN2_READ_HEADER(x) \
> > > >       u##x val = 0; \
> > > >       assert_rpm_wakelock_held(dev_priv);
> > > > @@ -892,6 +928,20 @@ static inline void
> > > > __force_wake_auto(struct
> > > > drm_i915_private *dev_priv,
> > > >           dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > > > fw_domains);
> > > >   }
> > > >   +static inline bool __is_forcewake_active(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > > +                     enum forcewake_domains fw_domains)
> > > > +{
> > > > +    struct intel_uncore_forcewake_domain *domain;
> > > > +
> > > > +    /* Ideally GCC would be constant-fold and eliminate this
> > > > loop */
> > > > +    for_each_fw_domain_masked(domain, fw_domains, dev_priv) {
> > > > +        if (domain->wake_count)
> > > > +            fw_domains &= ~domain->mask;
> > > > +    }
> > > > +
> > > > +    return fw_domains ? 0 : 1;
> > > > +}
> > > > +
> > > >   #define __gen6_read(x) \
> > > >   static u##x \
> > > >   gen6_read##x(struct drm_i915_private *dev_priv, i915_reg_t
> > > > reg, bool
> > > > trace) { \
> > > > @@ -940,6 +990,37 @@ gen9_read##x(struct drm_i915_private
> > > > *dev_priv,
> > > > i915_reg_t reg, bool trace) { \
> > > >       GEN6_READ_FOOTER; \
> > > >   }
> > > >   +#define __gen9_decoupled_read(x) \
> > > > +static u##x \
> > > > +gen9_decoupled_read##x(struct drm_i915_private *dev_priv,
> > > > i915_reg_t
> > > > reg, bool trace) { \
> > > > +    enum forcewake_domains fw_engine; \
> > > fw_engines
> > > 
> > > > 
> > > > +    GEN6_READ_HEADER(x); \
> > > > +    fw_engine = __gen9_reg_read_fw_domains(offset); \
> > > > +    if (fw_engine && x%32 == 0) { \
> > > > +        if (__is_forcewake_active(dev_priv, fw_engine)) \
> > > > +            __raw_i915_write##x(dev_priv, reg, val); \
> > > Write in the read macro, I don't understand!?
> > typo, I will fix it.
> > > 
> > > 
> > > Also, would a single mmio read call be possible, something like
> > > below?
> > > 
> > > if (x % 32 || !fw_engines || __is_forcewake_active()) {
> > >     if (fw_engines)
> > >         __force_wake_auto
> > >      __raw_i915_read
> > > } else {
> > >     ... decoupled mmio loop ...
> > > }
> > > 
> > > I might have made an oversight, no guarantees that I haven't. :)
> > Looks fine. Will test this
> Cool, the only thing which would still bug me here is the verboseness
> of 
> __is_forcewake_active but I have no smart ideas for that at the
> moment. 
> Perhaps keeping a bitmask of active domains in the core? Could be
> worth 
> it if for nothing for elegance.
> 
> Regards,
> 
> Tvrtko
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx