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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
