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

[Jani Nikula <jani.nikula@intel.com>]

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;
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx