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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com>

-- 
Ville Syrjälä
Intel