[Intel-gfx] [PATCH 2/2] drm/i915/i9xx_wm: Prefer intel_de functions over intel_uncore.

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel