| Message ID | 20190607120838.20514-8-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Legacy mmio accessor macro pruning | expand |
On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > 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@intel.com> 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_* 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 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. 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;
On 07/06/2019 14:11, Jani Nikula wrote: > On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> 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@intel.com> > > 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; >
On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 07/06/2019 14:11, Jani Nikula wrote: >> On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> 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@intel.com> >> >> 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? Yes, but haven't had the time to digest it. > 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. If there were display accessors they might just take i915 as pointer: #define FOO_READ(i915, reg) intel_uncore_read(&i915->display_uncore, reg) Dunno. > >> 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. Hum, you know what, it's not *that* big of a deal. Ack on the whole series, we can tidy up later on if needed. I guess I'd like to get Ville's ack on the series too. Ville? BR, Jani. > > 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; >>
On 07/06/2019 14:44, Jani Nikula wrote: > On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> On 07/06/2019 14:11, Jani Nikula wrote: >>> On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> 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@intel.com> >>> >>> 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? > > Yes, but haven't had the time to digest it. > >> 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. > > If there were display accessors they might just take i915 as pointer: > > #define FOO_READ(i915, reg) intel_uncore_read(&i915->display_uncore, reg) > > Dunno. > >> >>> 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. > > Hum, you know what, it's not *that* big of a deal. Ack on the whole > series, we can tidy up later on if needed. > > I guess I'd like to get Ville's ack on the series too. Ville? So I have patches 7 & 11 with acks from Jani and no r-b here. Depending on the opinion from Ville I can either ask for upgrade to r-b, or refactor them to minimize intel_uncore_read/write in favour of leaving the affected functions mixed (I915_READ/WRITE + intel_uncore_"more-exotic-accessors". Regards, Tvrtko
On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > 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@intel.com> > --- > 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); Just musing, a new intel_uncore_write_post() to combine the two? Reviewed-by: Jani Nikula <jani.nikula@intel.com> > /* 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;
On Tue, 11 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> Depending on the opinion from Ville I can either ask for upgrade to r-b,
r-b,
Jani.
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;