On Wed, Mar 08, 2023 at 08:18:52PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 08, 2023 at 12:56:28PM -0500, Rodrigo Vivi wrote:
> > 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?!
>
> Currently yes. Though I have occasionally thought about
> splitting it up lower down, since no display registers need
> forcewake, and IIRC the RM unclaimed stuff only really works
> for display registers. So we could perhaps lighten each
> side a bit by knowing ahead of time what kind of register
> we're dealing with.
>
> >
> > I believe i915/display should only call intel_de for mmio, so it
> > gets easier on the code reuse on Xe.
>
> Yeah, I get idea. However I think it might also be nice
> to check that we are not touching registers that we're
> not supposed to touch from the display code. So having
> intel_de*() validate the register offset might be nice.
> Would be especially important if we did do the lower
> level register accessor split.
>
> Though admittedly on these old platforms that valiation
> is perhaps a bit moot since the display vs. not split
> is far from clear, and even the truly dedicated display
> registers can live at rather weird offsets.
I guess we can always backpedal a bit if we do decide
to do those things. There shouldn't be that many non-display
registers in the mix anyway.
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
--
Ville Syrjälä
Intel
