On Wed, Feb 01, 2023 at 11:59:19AM +0200, Jani Nikula wrote: > On Tue, 31 Jan 2023, Lucas De Marchi <[email protected]> wrote: > > Instead of using the common DISPLAY_MMIO_BASE(dev_priv) in all single > > macros, only use them in the macros that are to be used outside the > > header. This reduces the use of the implicit dev_priv, making it easier > > to remove it later. > > > > Signed-off-by: Lucas De Marchi <[email protected]> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 73 ++++++++++++++++++--------------- > > 1 file changed, 39 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index 943db8ec63f8..1cde3bcb9c88 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -1182,9 +1182,9 @@ > > #define PM_VEBOX_CS_ERROR_INTERRUPT (1 << 12) /* hsw+ */ > > #define PM_VEBOX_USER_INTERRUPT (1 << 10) /* hsw+ */ > > > > -#define GT_PARITY_ERROR(dev_priv) \ > > +#define GT_PARITY_ERROR(__i915) \ > > (GT_RENDER_L3_PARITY_ERROR_INTERRUPT | \ > > - (IS_HASWELL(dev_priv) ? GT_RENDER_L3_PARITY_ERROR_INTERRUPT_S1 : 0)) > > + (IS_HASWELL(__i915) ? GT_RENDER_L3_PARITY_ERROR_INTERRUPT_S1 : 0)) > > Unrelated change. > > > > > /* These are all the "old" interrupts */ > > #define ILK_BSD_USER_INTERRUPT (1 << 5) > > @@ -1403,10 +1403,11 @@ > > /* > > * Clock control & power management > > */ > > -#define _DPLL_A (DISPLAY_MMIO_BASE(dev_priv) + 0x6014) > > -#define _DPLL_B (DISPLAY_MMIO_BASE(dev_priv) + 0x6018) > > -#define _CHV_DPLL_C (DISPLAY_MMIO_BASE(dev_priv) + 0x6030) > > -#define DPLL(pipe) _MMIO_PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C) > > +#define _DPLL_A 0x6014 > > +#define _DPLL_B 0x6018 > > +#define _CHV_DPLL_C 0x6030 > > +#define DPLL(pipe) _MMIO(DISPLAY_MMIO_BASE(dev_priv) + \ > > + _PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C)) > > > > #define VGA0 _MMIO(0x6000) > > #define VGA1 _MMIO(0x6004) > > @@ -1502,10 +1503,11 @@ > > #define SDVO_MULTIPLIER_SHIFT_HIRES 4 > > #define SDVO_MULTIPLIER_SHIFT_VGA 0 > > > > -#define _DPLL_A_MD (DISPLAY_MMIO_BASE(dev_priv) + 0x601c) > > -#define _DPLL_B_MD (DISPLAY_MMIO_BASE(dev_priv) + 0x6020) > > -#define _CHV_DPLL_C_MD (DISPLAY_MMIO_BASE(dev_priv) + 0x603c) > > -#define DPLL_MD(pipe) _MMIO_PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, > > _CHV_DPLL_C_MD) > > +#define _DPLL_A_MD 0x601c > > +#define _DPLL_B_MD 0x6020 > > +#define _CHV_DPLL_C_MD 0x603c > > +#define DPLL_MD(pipe) _MMIO(DISPLAY_MMIO_BASE(dev_priv) + \ > > + _PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, > > _CHV_DPLL_C_MD)) > > > > /* > > * UDI pixel divider, controlling how many pixels are stuffed into a > > packet. > > @@ -3323,42 +3325,45 @@ > > * is 20 bytes in each direction, hence the 5 fixed > > * data registers > > */ > > -#define _DPA_AUX_CH_CTL (DISPLAY_MMIO_BASE(dev_priv) + 0x64010) > > -#define _DPA_AUX_CH_DATA1 (DISPLAY_MMIO_BASE(dev_priv) + 0x64014) > > - > > -#define _DPB_AUX_CH_CTL (DISPLAY_MMIO_BASE(dev_priv) + 0x64110) > > -#define _DPB_AUX_CH_DATA1 (DISPLAY_MMIO_BASE(dev_priv) + 0x64114) > > - > > -#define DP_AUX_CH_CTL(aux_ch) _MMIO_PORT(aux_ch, _DPA_AUX_CH_CTL, > > _DPB_AUX_CH_CTL) > > -#define DP_AUX_CH_DATA(aux_ch, i) _MMIO(_PORT(aux_ch, _DPA_AUX_CH_DATA1, > > _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */ > > +#define _DPA_AUX_CH_CTL 0x64010 > > +#define _DPA_AUX_CH_DATA1 0x64014 > > +#define _DPB_AUX_CH_CTL 0x64110 > > +#define _DPB_AUX_CH_DATA1 0x64114 > > +#define DP_AUX_CH_CTL(aux_ch) _MMIO(DISPLAY_MMIO_BASE(dev_priv) + \ > > + _PORT(aux_ch, _DPA_AUX_CH_CTL, > > _DPB_AUX_CH_CTL)) > > +#define DP_AUX_CH_DATA(aux_ch, i) \ > > + _MMIO(DISPLAY_MMIO_BASE(dev_priv) + \ > > + _PORT(aux_ch, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* > > 5 registers */ > > > > #define _XELPDP_USBC1_AUX_CH_CTL 0x16F210 > > #define _XELPDP_USBC2_AUX_CH_CTL 0x16F410 > > #define _XELPDP_USBC3_AUX_CH_CTL 0x16F610 > > #define _XELPDP_USBC4_AUX_CH_CTL 0x16F810 > > > > -#define XELPDP_DP_AUX_CH_CTL(aux_ch) _MMIO(_PICK(aux_ch, \ > > - _DPA_AUX_CH_CTL, \ > > - _DPB_AUX_CH_CTL, \ > > - 0, /* port/aux_ch C is > > non-existent */ \ > > - > > _XELPDP_USBC1_AUX_CH_CTL, \ > > - > > _XELPDP_USBC2_AUX_CH_CTL, \ > > - > > _XELPDP_USBC3_AUX_CH_CTL, \ > > - > > _XELPDP_USBC4_AUX_CH_CTL)) > > +#define XELPDP_DP_AUX_CH_CTL(aux_ch) > > _MMIO(DISPLAY_MMIO_BASE(dev_priv) + \ > > Note that only VLV and CHV have DISPLAY_MMIO_BASE() != 0. > > This is an XELPDP specific macro. Just drop the DISPLAY_MMIO_BASE() part > altogether, and you've removed an implicit dev_priv. Yay. > > This also makes me think we should probably add VLV/CHV specific > DP_AUX_CH_CTL and DP_AUX_CH_DATA macros that just add VLV_DISPLAY_BASE > directly, and use that to ditch the implicit dev_priv there too. This > approach doesn't work for everything, but the aux channel stuff is both > fairly limited use and already has if ladders to pick the > registers. Handling VLV/CHV separately is not a big deal. > > (DPLL on the other hand seems much harder to deal that way.)
Most of the DPLL code should be have vlv/chv vs. rest specific codepaths. A quick scan says readout is perhaps the only exception. -- Ville Syrjälä Intel
