[Intel-gfx] [PATCH v16 3/7] drm/i915: Introduce parameterized DBUF_CTL
Lisovskiy, Stanislav
stanislav.lisovskiy at intel.com
Wed Jan 29 08:41:34 UTC 2020
On Tue, 2020-01-28 at 19:35 +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2020 at 10:44:52AM +0200, Stanislav Lisovskiy wrote:
> > Now start using parameterized DBUF_CTL instead
> > of hardcoded, this would allow shorter access
> > functions when reading or storing entire state.
> >
> > Tried to implement it in a MMIO_PIPE manner, however
> > DBUF_CTL1 address is higher than DBUF_CTL2, which
> > implies that we have to now subtract from base
> > rather than add.
> >
> > v2: - Removed unneeded DBUF_CTL_DIST and DBUF_CTL_ADDR
> > macros. Started to use _PICK construct as suggested
> > by Matt Roper.
> >
> > v3: - DBUF_CTL_S* to _DBUF_CTL_S*, changed X to "slice"
> > in macro(Ville Syrjälä)
> > - Introduced enum for enumerating DBUF slices(Ville Syrjälä)
> >
> > Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > ---
> > .../drm/i915/display/intel_display_power.c | 30 +++++++++++--
> > ------
> > .../drm/i915/display/intel_display_power.h | 5 ++++
> > drivers/gpu/drm/i915/i915_reg.h | 7 +++--
> > drivers/gpu/drm/i915/intel_pm.c | 2 +-
> > 4 files changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 5e1c601f0f99..a59efb24be92 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -4418,9 +4418,11 @@ void icl_dbuf_slices_update(struct
> > drm_i915_private *dev_priv,
> > return;
> >
> > if (req_slices > hw_enabled_slices)
> > - ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2,
> > true);
> > + ret = intel_dbuf_slice_set(dev_priv,
> > + _DBUF_CTL_S(DBUF_S2), true);
> > else
> > - ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2,
> > false);
> > + ret = intel_dbuf_slice_set(dev_priv,
> > + _DBUF_CTL_S(DBUF_S2),
> > false);
> >
> > if (ret)
> > dev_priv->enabled_dbuf_slices_num = req_slices;
> > @@ -4428,14 +4430,16 @@ void icl_dbuf_slices_update(struct
> > drm_i915_private *dev_priv,
> >
> > static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
> > {
> > - I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) |
> > DBUF_POWER_REQUEST);
> > - I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) |
> > DBUF_POWER_REQUEST);
> > - POSTING_READ(DBUF_CTL_S2);
> > + I915_WRITE(_DBUF_CTL_S(DBUF_S1),
> > + I915_READ(_DBUF_CTL_S(DBUF_S1)) |
> > DBUF_POWER_REQUEST);
> > + I915_WRITE(_DBUF_CTL_S(DBUF_S2),
> > + I915_READ(_DBUF_CTL_S(DBUF_S2)) |
> > DBUF_POWER_REQUEST);
> > + POSTING_READ(_DBUF_CTL_S(DBUF_S2));
> >
> > udelay(10);
> >
> > - if (!(I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
> > - !(I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
> > + if (!(I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
> > + !(I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
> > DRM_ERROR("DBuf power enable timeout\n");
> > else
> > /*
> > @@ -4447,14 +4451,16 @@ static void icl_dbuf_enable(struct
> > drm_i915_private *dev_priv)
> >
> > static void icl_dbuf_disable(struct drm_i915_private *dev_priv)
> > {
> > - I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) &
> > ~DBUF_POWER_REQUEST);
> > - I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) &
> > ~DBUF_POWER_REQUEST);
> > - POSTING_READ(DBUF_CTL_S2);
> > + I915_WRITE(_DBUF_CTL_S(DBUF_S1),
> > + I915_READ(_DBUF_CTL_S(DBUF_S1)) &
> > ~DBUF_POWER_REQUEST);
> > + I915_WRITE(_DBUF_CTL_S(DBUF_S2),
> > + I915_READ(_DBUF_CTL_S(DBUF_S2)) &
> > ~DBUF_POWER_REQUEST);
> > + POSTING_READ(_DBUF_CTL_S(DBUF_S2));
> >
> > udelay(10);
> >
> > - if ((I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
> > - (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
> > + if ((I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
> > + (I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
> > DRM_ERROR("DBuf power disable timeout!\n");
> > else
> > /*
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h
> > b/drivers/gpu/drm/i915/display/intel_display_power.h
> > index 2608a65af7fa..601e000ffd0d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> > @@ -307,6 +307,11 @@ intel_display_power_put_async(struct
> > drm_i915_private *i915,
> > }
> > #endif
> >
> > +enum dbuf_slice {
> > + DBUF_S1,
> > + DBUF_S2,
> > +};
> > +
> > #define with_intel_display_power(i915, domain, wf) \
> > for ((wf) = intel_display_power_get((i915), (domain)); (wf); \
> > intel_display_power_put_async((i915), (domain), (wf)),
> > (wf) = 0)
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index b93c4c18f05c..625be54d3eae 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7748,9 +7748,10 @@ enum {
> > #define DISP_ARB_CTL2 _MMIO(0x45004)
> > #define DISP_DATA_PARTITION_5_6 (1 << 6)
> > #define DISP_IPC_ENABLE (1 << 3)
> > -#define DBUF_CTL _MMIO(0x45008)
> > -#define DBUF_CTL_S1 _MMIO(0x45008)
> > -#define DBUF_CTL_S2 _MMIO(0x44FE8)
> > +#define DBUF_CTL_ADDR1 0x45008
> > +#define DBUF_CTL_ADDR2 0x44FE8
> > +#define _DBUF_CTL_S(X) _MMIO(_PICK_EVEN(X,
> > DBUF_CTL_ADDR1, DBUF_CTL_ADDR2))
>
> That's not at all what I meant. Also the 'X' is still there despite
> what
> the changelog says.
>
> #define _DBUF_CTL_S1 0x45008
> #define _DBUF_CTL_S2 0x44FE8
> #define DBUF_CTL_S(slice) _MMIO(_PICK_EVEN(slice, _DBUF_CTL_S1,
> _DBUF_CTL_S2))
My idea was to still be able to use DBUF_CTL_S1 and DBUF_CTL_S2, of
course this is a bit redundant, but though similar naming DBUF_CTL_S1
and DBUF_CTL_S(0) might confuse somebody into using it.
For example in your case we can now only use DBUF_CTL_S() macro because
_DBUF_CTL_S1/2 is not any longer using _MMIO.
But _now_ I get your point, I guess by "_" in the beginning, it is
meant not to be used from outside. Some kind of like private class
members in Python :)
I will change this once the rest of patches are reviewed, because that
change anyway does not affect the actual functionality, but purely
cosmetic.
Stan
>
>
> > +#define DBUF_CTL _DBUF_CTL_S(0)
> > #define DBUF_POWER_REQUEST (1 << 31)
> > #define DBUF_POWER_STATE (1 << 30)
> > #define GEN7_MSG_CTL _MMIO(0x45010)
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 04f94057d6b3..b8d78e26515c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3660,7 +3660,7 @@ u8 intel_enabled_dbuf_slices_num(struct
> > drm_i915_private *dev_priv)
> > * only that 1 slice enabled until we have a proper way for on-
> > demand
> > * toggling of the second slice.
> > */
> > - if (0 && I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)
> > + if (0 && I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE)
> > enabled_dbuf_slices_num++;
> >
> > return enabled_dbuf_slices_num;
> > --
> > 2.24.1.485.gad05a3d8e5
>
>
More information about the Intel-gfx
mailing list