On Wed, Mar 08, 2023 at 07:50:27PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 08, 2023 at 11:58:59AM -0500, Rodrigo Vivi wrote:
> > } else if (IS_I915GM(dev_priv)) {
> > /*
> > * FIXME can't find a bit like this for 915G, and
> > * yet it does have the related watermark in
> > * FW_BLC_SELF. What's going on?
> > */
> > - was_enabled = intel_uncore_read(&dev_priv->uncore, INSTPM) & INSTPM_SELF_EN;
> > + was_enabled = intel_de_read(dev_priv, INSTPM) & INSTPM_SELF_EN;
> > val = enable ? _MASKED_BIT_ENABLE(INSTPM_SELF_EN) :
> > _MASKED_BIT_DISABLE(INSTPM_SELF_EN);
> > - intel_uncore_write(&dev_priv->uncore, INSTPM, val);
> > - intel_uncore_posting_read(&dev_priv->uncore, INSTPM);
> > + intel_de_write(dev_priv, INSTPM, val);
> > + intel_de_posting_read(dev_priv, INSTPM);
>
> I'm still not really convinced that we want to
> use intel_de_*() for non-display registers.
hmmm... I see...
so should we create a new component out of i915/display and move
these calls there?
but in the end of the day it is the same uncore functions that
are getting calling underneath anyway, right?!
I believe i915/display should only call intel_de for mmio, so it
gets easier on the code reuse on Xe.
>
> --
> Ville Syrjälä
> Intel
