Re: [RFC 07/12] drm/i915: Remove I915_READ8

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 07/06/2019 14:11, Jani Nikula wrote:
> On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >
> > Only a few call sites remain which have been converted to uncore mmio
> > accessors and so the macro can be removed.
> >
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> I have some reservations about this one and patch 11. Again, I'm fine
> with nuking I915_READ8 and I915_READ_NOTRACE (in patch 11). I think
> they're special cases and it's okay if they grow into a bit longer lines
> or multiple lines.
>
> The problem is converting regular I915_READ and I915_WRITE in display
> code while at it.
>
> I don't think en masse conversion of them to intel_uncore_read and
> intel_uncore_write directly is going to happen in display code, because
> there's too much code that gets turned too ugly with the increase in
> function name length and the extra passed parameter. I think many of
> those places are pretty ugly as it is already. That's my feeling anyway.
>
> I understand your reasoning is to avoid the mixed use of intel_uncore_*

Exactly.

> and I915_*. But I fear using intel_uncore_read and intel_uncore_write
> now is going to promote their use all over the place, and *that* will
> lead to mixed use. Which is not optimal if the overall feeling is that
> we need to come up with a shorter function/macro for display code reads
> and writes.

I am fine with the argument that you may desire something shorter for 
display. But I don't think converting whole functions would create any 
more future mixed use than converting functions partially.

Btw have you seen the latest RFC from Daniele? That would allow that you 
only replace the assignemnts at the top of functions perhaps like from:

	struct intel_uncore *uncore = &dev_priv->uncore;

to:

	struct intel_uncore *uncore = &dev_priv->display_uncore;

But sure, if you desire shorter accessors then lets first have a 
definitive decision of that.

> I presume Ville has something to say about the gt vs. display stuff as
> well; does the whole series make that harder? I hope not, because I do
> like the rest of what's being done here.

It doesn't make it harder. I can easily drop the bits you don't like if 
that will be the final decision. In fact, I should probably do that 
straight away if that would unblock the remaining two patches because 
then I can proceed with other refactorings on top.

Regards,

Tvrtko

> BR,
> Jani.
>
>
>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h  |  2 --
> >   drivers/gpu/drm/i915/intel_crt.c | 41 ++++++++++++++++++--------------
> >   drivers/gpu/drm/i915/intel_pm.c  |  6 ++---
> >   3 files changed, 26 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index b2763721b76d..13815795e197 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2852,8 +2852,6 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
> >   #define __I915_REG_OP(op__, dev_priv__, ...) \
> >   	intel_uncore_##op__(&(dev_priv__)->uncore, __VA_ARGS__)
> >   
> > -#define I915_READ8(reg__)	  __I915_REG_OP(read8, dev_priv, (reg__))
> > -
> >   #define I915_READ16(reg__)	   __I915_REG_OP(read16, dev_priv, (reg__))
> >   #define I915_WRITE16(reg__, val__) __I915_REG_OP(write16, dev_priv, (reg__), (val__))
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> > index bb56518576a1..3fcf2f84bcce 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -643,6 +643,7 @@ intel_crt_load_detect(struct intel_crt *crt, u32 pipe)
> >   {
> >   	struct drm_device *dev = crt->base.base.dev;
> >   	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct intel_uncore *uncore = &dev_priv->uncore;
> >   	u32 save_bclrpat;
> >   	u32 save_vtotal;
> >   	u32 vtotal, vactive;
> > @@ -663,9 +664,9 @@ intel_crt_load_detect(struct intel_crt *crt, u32 pipe)
> >   	pipeconf_reg = PIPECONF(pipe);
> >   	pipe_dsl_reg = PIPEDSL(pipe);
> >   
> > -	save_bclrpat = I915_READ(bclrpat_reg);
> > -	save_vtotal = I915_READ(vtotal_reg);
> > -	vblank = I915_READ(vblank_reg);
> > +	save_bclrpat = intel_uncore_read(uncore, bclrpat_reg);
> > +	save_vtotal = intel_uncore_read(uncore, vtotal_reg);
> > +	vblank = intel_uncore_read(uncore, vblank_reg);
> >   
> >   	vtotal = ((save_vtotal >> 16) & 0xfff) + 1;
> >   	vactive = (save_vtotal & 0x7ff) + 1;
> > @@ -674,21 +675,23 @@ intel_crt_load_detect(struct intel_crt *crt, u32 pipe)
> >   	vblank_end = ((vblank >> 16) & 0xfff) + 1;
> >   
> >   	/* Set the border color to purple. */
> > -	I915_WRITE(bclrpat_reg, 0x500050);
> > +	intel_uncore_write(uncore, bclrpat_reg, 0x500050);
> >   
> >   	if (!IS_GEN(dev_priv, 2)) {
> > -		u32 pipeconf = I915_READ(pipeconf_reg);
> > -		I915_WRITE(pipeconf_reg, pipeconf | PIPECONF_FORCE_BORDER);
> > -		POSTING_READ(pipeconf_reg);
> > +		u32 pipeconf = intel_uncore_read(uncore, pipeconf_reg);
> > +		intel_uncore_write(uncore,
> > +				   pipeconf_reg,
> > +				   pipeconf | PIPECONF_FORCE_BORDER);
> > +		intel_uncore_posting_read(uncore, pipeconf_reg);
> >   		/* Wait for next Vblank to substitue
> >   		 * border color for Color info */
> >   		intel_wait_for_vblank(dev_priv, pipe);
> > -		st00 = I915_READ8(_VGA_MSR_WRITE);
> > +		st00 = intel_uncore_read8(uncore, _VGA_MSR_WRITE);
> >   		status = ((st00 & (1 << 4)) != 0) ?
> >   			connector_status_connected :
> >   			connector_status_disconnected;
> >   
> > -		I915_WRITE(pipeconf_reg, pipeconf);
> > +		intel_uncore_write(uncore, pipeconf_reg, pipeconf);
> >   	} else {
> >   		bool restore_vblank = false;
> >   		int count, detect;
> > @@ -702,9 +705,10 @@ intel_crt_load_detect(struct intel_crt *crt, u32 pipe)
> >   			u32 vsync_start = (vsync & 0xffff) + 1;
> >   
> >   			vblank_start = vsync_start;
> > -			I915_WRITE(vblank_reg,
> > -				   (vblank_start - 1) |
> > -				   ((vblank_end - 1) << 16));
> > +			intel_uncore_write(uncore,
> > +					   vblank_reg,
> > +					   (vblank_start - 1) |
> > +					   ((vblank_end - 1) << 16));
> >   			restore_vblank = true;
> >   		}
> >   		/* sample in the vertical border, selecting the larger one */
> > @@ -716,9 +720,10 @@ intel_crt_load_detect(struct intel_crt *crt, u32 pipe)
> >   		/*
> >   		 * Wait for the border to be displayed
> >   		 */
> > -		while (I915_READ(pipe_dsl_reg) >= vactive)
> > +		while (intel_uncore_read(uncore, pipe_dsl_reg) >= vactive)
> >   			;
> > -		while ((dsl = I915_READ(pipe_dsl_reg)) <= vsample)
> > +		while ((dsl = intel_uncore_read(uncore, pipe_dsl_reg)) <=
> > +		       vsample)
> >   			;
> >   		/*
> >   		 * Watch ST00 for an entire scanline
> > @@ -728,14 +733,14 @@ intel_crt_load_detect(struct intel_crt *crt, u32 pipe)
> >   		do {
> >   			count++;
> >   			/* Read the ST00 VGA status register */
> > -			st00 = I915_READ8(_VGA_MSR_WRITE);
> > +			st00 = intel_uncore_read8(uncore, _VGA_MSR_WRITE);
> >   			if (st00 & (1 << 4))
> >   				detect++;
> > -		} while ((I915_READ(pipe_dsl_reg) == dsl));
> > +		} while ((intel_uncore_read(uncore, pipe_dsl_reg) == dsl));
> >   
> >   		/* restore vblank if necessary */
> >   		if (restore_vblank)
> > -			I915_WRITE(vblank_reg, vblank);
> > +			intel_uncore_write(uncore, vblank_reg, vblank);
> >   		/*
> >   		 * If more than 3/4 of the scanline detected a monitor,
> >   		 * then it is assumed to be present. This works even on i830,
> > @@ -748,7 +753,7 @@ intel_crt_load_detect(struct intel_crt *crt, u32 pipe)
> >   	}
> >   
> >   	/* Restore previous settings */
> > -	I915_WRITE(bclrpat_reg, save_bclrpat);
> > +	intel_uncore_write(uncore, bclrpat_reg, save_bclrpat);
> >   
> >   	return status;
> >   }
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index d7272d4ff258..93e411e6ad19 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -8160,15 +8160,15 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
> >   	return val;
> >   }
> >   
> > -unsigned long i915_mch_val(struct drm_i915_private *dev_priv)
> > +unsigned long i915_mch_val(struct drm_i915_private *i915)
> >   {
> >   	unsigned long m, x, b;
> >   	u32 tsfs;
> >   
> > -	tsfs = I915_READ(TSFS);
> > +	tsfs = intel_uncore_read(&i915->uncore, TSFS);
> >   
> >   	m = ((tsfs & TSFS_SLOPE_MASK) >> TSFS_SLOPE_SHIFT);
> > -	x = I915_READ8(TR1);
> > +	x = intel_uncore_read8(&i915->uncore, TR1);
> >   
> >   	b = tsfs & TSFS_INTR_MASK;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx