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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux