On Fri, 07 Jun 2019, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> 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?
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;
>>
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
