| Message ID | 20190808014423.20377-1-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Display uncore | expand |
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. 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. -Chris
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. BR, Jani.
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. >>> >>>
On Wed, 07 Aug 2019, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: > 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. > > Thoughts? I'm very open to (and I actually hope for) better ideas. Has there been any progress in this front lately, or have I just missed it? BR, Jani.
On 10/29/19 2:23 AM, Jani Nikula wrote: > On Wed, 07 Aug 2019, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: >> 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. >> >> Thoughts? I'm very open to (and I actually hope for) better ideas. > > Has there been any progress in this front lately, or have I just missed > it? > No progress on the ML. I've been locally trying on and off to break i915_reg.h in more manageable chunks to be able to more easily mark the display registers, but I keep finding special cases, especially around VLV/CHV. I'll try to prioritize this task more and get something out, at least as a RFC. Daniele > BR, > Jani. >
On Tue, 29 Oct 2019, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: > On 10/29/19 2:23 AM, Jani Nikula wrote: >> On Wed, 07 Aug 2019, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: >>> 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. >>> >>> Thoughts? I'm very open to (and I actually hope for) better ideas. >> >> Has there been any progress in this front lately, or have I just missed >> it? >> > > No progress on the ML. I've been locally trying on and off to break > i915_reg.h in more manageable chunks to be able to more easily mark the > display registers, but I keep finding special cases, especially around > VLV/CHV. I'll try to prioritize this task more and get something out, at > least as a RFC. Okay. As you saw, I decided to start moving forward with display uncore helpers [1]. We'll need them anyway, and we can add the display checks there afterwards. If anything, they should make your job easier, not harder. BR, Jani. [1] http://patchwork.freedesktop.org/patch/msgid/20191029105156.21658-1-jani.nikula@intel.com