[RFC,0/3] Display uncore

Submitted by Jani Nikula on Aug. 8, 2019, 1:58 p.m.

Details

Message ID 87ef1viws2.fsf@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Jani Nikula Aug. 8, 2019, 1:58 p.m.
On Thu, 08 Aug 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Daniele Ceraolo Spurio (2019-08-08 02:44:20)
>> I've been trying to identify MMIO ranges to clearly define what belongs
>> to display_uncore to do a check on access, but there are lots of
>> exceptions and differences across gens (with a few more coming with TGL),
>> so I don't think that's a viable way. The alternative option implemented
>> here is to differentiate the register by type, which should ensure we
>> never mix them up, but at the cost of a more complex transition.
>
> One thing we could try with this approach is to tag every _MMIO() as
> either DE or GT and have the uncore accessors check the magic bits
> before scrubbing them. (With enough magic macros to make it disappear
>
> Huge task, but not insurmountable. The danger is that if we do this
> piecemeal is that we may end up with two simultaneous accesses via the
> separate uncore accessors. Hmm.

You mean something like this?

---

bloat-o-meter tells me just that, with no other changes is +0.53%
increase. Perhaps still acceptable.

I think we could just add something like

#define _MMIO_DE(r) ((const i915_reg_t){ .de = 1, .reg = (r) })

and update i915_reg.h to use that as the first step, with no other
changes, and build on top of that. I don't think there should be large
scale changes outside of i915_reg.h required at all at first. The update
to move away from I915_READ and I915_WRITE could come afterwards and
piecemeal AFAICT.

> On thing though is that Jani may find the intel_de_write (or just
> de_write, the frequency may be worth bending the naming rules) as being
> palatable.

Indeed. Already intel_de_write(i915, ...) is so much better than
intel_uncore_write(&i915->uncore, ...).

Though... with de encoded in i915_reg_t, we could have intel_write(i915,
...) do the right thing based on .de. It could internally choose the
right uncore for intel_uncore_write(). Even if most non-de would
directly use the uncore versions.

BR,
Jani.

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b362ca0663a6..401490f79935 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -179,7 +179,8 @@ 
 #define REG_FIELD_GET(__mask, __val)	((u32)FIELD_GET(__mask, __val))
 
 typedef struct {
-	u32 reg;
+	u32 de:1;
+	u32 reg:31;
 } i915_reg_t;
 
 #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })

Comments

On 8/8/19 6:58 AM, Jani Nikula wrote:
> On Thu, 08 Aug 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> Quoting Daniele Ceraolo Spurio (2019-08-08 02:44:20)
>>> I've been trying to identify MMIO ranges to clearly define what belongs
>>> to display_uncore to do a check on access, but there are lots of
>>> exceptions and differences across gens (with a few more coming with TGL),
>>> so I don't think that's a viable way. The alternative option implemented
>>> here is to differentiate the register by type, which should ensure we
>>> never mix them up, but at the cost of a more complex transition.
>>
>> One thing we could try with this approach is to tag every _MMIO() as
>> either DE or GT and have the uncore accessors check the magic bits
>> before scrubbing them. (With enough magic macros to make it disappear
>>
>> Huge task, but not insurmountable. The danger is that if we do this
>> piecemeal is that we may end up with two simultaneous accesses via the
>> separate uncore accessors. Hmm.
> 
> You mean something like this?
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b362ca0663a6..401490f79935 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -179,7 +179,8 @@
>   #define REG_FIELD_GET(__mask, __val)	((u32)FIELD_GET(__mask, __val))
>   
>   typedef struct {
> -	u32 reg;
> +	u32 de:1;
> +	u32 reg:31;
>   } i915_reg_t;
>   
>   #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
> ---
> 
> bloat-o-meter tells me just that, with no other changes is +0.53%
> increase. Perhaps still acceptable.
> 
> I think we could just add something like
> 
> #define _MMIO_DE(r) ((const i915_reg_t){ .de = 1, .reg = (r) })
> 
> and update i915_reg.h to use that as the first step, with no other
> changes, and build on top of that. I don't think there should be large
> scale changes outside of i915_reg.h required at all at first. The update
> to move away from I915_READ and I915_WRITE could come afterwards and
> piecemeal AFAICT.
> 
>> On thing though is that Jani may find the intel_de_write (or just
>> de_write, the frequency may be worth bending the naming rules) as being
>> palatable.
> 
> Indeed. Already intel_de_write(i915, ...) is so much better than
> intel_uncore_write(&i915->uncore, ...).
> 
> Though... with de encoded in i915_reg_t, we could have intel_write(i915,
> ...) do the right thing based on .de. It could internally choose the
> right uncore for intel_uncore_write(). Even if most non-de would
> directly use the uncore versions.
> 

I'd prefer to avoid the implicit selection in the new functions, but, 
for compatibility during the transition, we could add the selection 
inside the old I915_READ/WRITE() calls. This way we'll be able to ensure 
that even the non yet converted accesses will go through the correct uncore.

Daniele

> BR,
> Jani.
> 
>
On Thu, Aug 08, 2019 at 09:47:56AM -0700, Daniele Ceraolo Spurio wrote:
>
>
>On 8/8/19 6:58 AM, Jani Nikula wrote:
>>On Thu, 08 Aug 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>Quoting Daniele Ceraolo Spurio (2019-08-08 02:44:20)
>>>>I've been trying to identify MMIO ranges to clearly define what belongs
>>>>to display_uncore to do a check on access, but there are lots of
>>>>exceptions and differences across gens (with a few more coming with TGL),
>>>>so I don't think that's a viable way. The alternative option implemented
>>>>here is to differentiate the register by type, which should ensure we
>>>>never mix them up, but at the cost of a more complex transition.
>>>
>>>One thing we could try with this approach is to tag every _MMIO() as
>>>either DE or GT and have the uncore accessors check the magic bits
>>>before scrubbing them. (With enough magic macros to make it disappear
>>>
>>>Huge task, but not insurmountable. The danger is that if we do this
>>>piecemeal is that we may end up with two simultaneous accesses via the
>>>separate uncore accessors. Hmm.
>>
>>You mean something like this?
>>
>>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>index b362ca0663a6..401490f79935 100644
>>--- a/drivers/gpu/drm/i915/i915_reg.h
>>+++ b/drivers/gpu/drm/i915/i915_reg.h
>>@@ -179,7 +179,8 @@
>>  #define REG_FIELD_GET(__mask, __val)	((u32)FIELD_GET(__mask, __val))
>>  typedef struct {
>>-	u32 reg;
>>+	u32 de:1;
>>+	u32 reg:31;
>>  } i915_reg_t;
>>  #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
>>---
>>
>>bloat-o-meter tells me just that, with no other changes is +0.53%
>>increase. Perhaps still acceptable.
>>
>>I think we could just add something like
>>
>>#define _MMIO_DE(r) ((const i915_reg_t){ .de = 1, .reg = (r) })
>>
>>and update i915_reg.h to use that as the first step, with no other
>>changes, and build on top of that. I don't think there should be large
>>scale changes outside of i915_reg.h required at all at first. The update
>>to move away from I915_READ and I915_WRITE could come afterwards and
>>piecemeal AFAICT.
>>
>>>On thing though is that Jani may find the intel_de_write (or just
>>>de_write, the frequency may be worth bending the naming rules) as being
>>>palatable.
>>
>>Indeed. Already intel_de_write(i915, ...) is so much better than
>>intel_uncore_write(&i915->uncore, ...).
>>
>>Though... with de encoded in i915_reg_t, we could have intel_write(i915,
>>...) do the right thing based on .de. It could internally choose the
>>right uncore for intel_uncore_write(). Even if most non-de would
>>directly use the uncore versions.
>>
>
>I'd prefer to avoid the implicit selection in the new functions, but, 
>for compatibility during the transition, we could add the selection 
>inside the old I915_READ/WRITE() calls. This way we'll be able to 
>ensure that even the non yet converted accesses will go through the 
>correct uncore.

Yeah, I'm with you on this one. For new functions I think it's better to
be explicit.

Lucas De Marchi

>
>Daniele
>
>>BR,
>>Jani.
>>
>>
On Thu, 08 Aug 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Thu, Aug 08, 2019 at 09:47:56AM -0700, Daniele Ceraolo Spurio wrote:
>>
>>
>>On 8/8/19 6:58 AM, Jani Nikula wrote:
>>>On Thu, 08 Aug 2019, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>>>Quoting Daniele Ceraolo Spurio (2019-08-08 02:44:20)
>>>>>I've been trying to identify MMIO ranges to clearly define what belongs
>>>>>to display_uncore to do a check on access, but there are lots of
>>>>>exceptions and differences across gens (with a few more coming with TGL),
>>>>>so I don't think that's a viable way. The alternative option implemented
>>>>>here is to differentiate the register by type, which should ensure we
>>>>>never mix them up, but at the cost of a more complex transition.
>>>>
>>>>One thing we could try with this approach is to tag every _MMIO() as
>>>>either DE or GT and have the uncore accessors check the magic bits
>>>>before scrubbing them. (With enough magic macros to make it disappear
>>>>
>>>>Huge task, but not insurmountable. The danger is that if we do this
>>>>piecemeal is that we may end up with two simultaneous accesses via the
>>>>separate uncore accessors. Hmm.
>>>
>>>You mean something like this?
>>>
>>>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>index b362ca0663a6..401490f79935 100644
>>>--- a/drivers/gpu/drm/i915/i915_reg.h
>>>+++ b/drivers/gpu/drm/i915/i915_reg.h
>>>@@ -179,7 +179,8 @@
>>>  #define REG_FIELD_GET(__mask, __val)	((u32)FIELD_GET(__mask, __val))
>>>  typedef struct {
>>>-	u32 reg;
>>>+	u32 de:1;
>>>+	u32 reg:31;
>>>  } i915_reg_t;
>>>  #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
>>>---
>>>
>>>bloat-o-meter tells me just that, with no other changes is +0.53%
>>>increase. Perhaps still acceptable.
>>>
>>>I think we could just add something like
>>>
>>>#define _MMIO_DE(r) ((const i915_reg_t){ .de = 1, .reg = (r) })
>>>
>>>and update i915_reg.h to use that as the first step, with no other
>>>changes, and build on top of that. I don't think there should be large
>>>scale changes outside of i915_reg.h required at all at first. The update
>>>to move away from I915_READ and I915_WRITE could come afterwards and
>>>piecemeal AFAICT.
>>>
>>>>On thing though is that Jani may find the intel_de_write (or just
>>>>de_write, the frequency may be worth bending the naming rules) as being
>>>>palatable.
>>>
>>>Indeed. Already intel_de_write(i915, ...) is so much better than
>>>intel_uncore_write(&i915->uncore, ...).
>>>
>>>Though... with de encoded in i915_reg_t, we could have intel_write(i915,
>>>...) do the right thing based on .de. It could internally choose the
>>>right uncore for intel_uncore_write(). Even if most non-de would
>>>directly use the uncore versions.
>>>
>>
>>I'd prefer to avoid the implicit selection in the new functions, but, 
>>for compatibility during the transition, we could add the selection 
>>inside the old I915_READ/WRITE() calls. This way we'll be able to 
>>ensure that even the non yet converted accesses will go through the 
>>correct uncore.
>
> Yeah, I'm with you on this one. For new functions I think it's better to
> be explicit.

I suppose my question is, how is it implicit if it's explicitly
specified in the i915_reg_t?

BR,
Jani.


>
> Lucas De Marchi
>
>>
>>Daniele
>>
>>>BR,
>>>Jani.
>>>
>>>