| Message ID | 20230131191542.1695398-2-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Split _MMIO() for _PORT3()/_PIPE3() | expand |
On Tue, 31 Jan 2023, Lucas De Marchi <lucas.demarchi@intel.com> 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 <lucas.demarchi@intel.com> > --- > 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.) BR, Jani. > + _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_USBC1_AUX_CH_DATA1 0x16F214 > #define _XELPDP_USBC2_AUX_CH_DATA1 0x16F414 > #define _XELPDP_USBC3_AUX_CH_DATA1 0x16F614 > #define _XELPDP_USBC4_AUX_CH_DATA1 0x16F814 > > -#define XELPDP_DP_AUX_CH_DATA(aux_ch, i) _MMIO(_PICK(aux_ch, \ > - _DPA_AUX_CH_DATA1, \ > - _DPB_AUX_CH_DATA1, \ > - 0, /* port/aux_ch C is non-existent */ \ > - _XELPDP_USBC1_AUX_CH_DATA1, \ > - _XELPDP_USBC2_AUX_CH_DATA1, \ > - _XELPDP_USBC3_AUX_CH_DATA1, \ > - _XELPDP_USBC4_AUX_CH_DATA1) + (i) * 4) > +#define XELPDP_DP_AUX_CH_DATA(aux_ch, i) _MMIO(DISPLAY_MMIO_BASE(dev_priv) + \ > + _PICK(aux_ch, \ > + _DPA_AUX_CH_DATA1, \ > + _DPB_AUX_CH_DATA1, \ > + 0, /* port/aux_ch C is non-existent */ \ > + _XELPDP_USBC1_AUX_CH_DATA1, \ > + _XELPDP_USBC2_AUX_CH_DATA1, \ > + _XELPDP_USBC3_AUX_CH_DATA1, \ > + _XELPDP_USBC4_AUX_CH_DATA1) + (i) * 4) > > #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31) > #define DP_AUX_CH_CTL_DONE (1 << 30)
On Wed, Feb 01, 2023 at 11:59:19AM +0200, Jani Nikula wrote: > On Tue, 31 Jan 2023, Lucas De Marchi <lucas.demarchi@intel.com> 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 <lucas.demarchi@intel.com> > > --- > > 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.
On Wed, Feb 01, 2023 at 02:09:54PM +0200, Ville Syrjälä wrote: >On Wed, Feb 01, 2023 at 11:59:19AM +0200, Jani Nikula wrote: >> On Tue, 31 Jan 2023, Lucas De Marchi <lucas.demarchi@intel.com> 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 <lucas.demarchi@intel.com> >> > --- >> > 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. and then remove display.mmio_offset since vlv/chv are the only ones using it? Lucas De Marchi
On Wed, Feb 01, 2023 at 09:26:29AM -0800, Lucas De Marchi wrote: > On Wed, Feb 01, 2023 at 02:09:54PM +0200, Ville Syrjälä wrote: > >On Wed, Feb 01, 2023 at 11:59:19AM +0200, Jani Nikula wrote: > >> On Tue, 31 Jan 2023, Lucas De Marchi <lucas.demarchi@intel.com> 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 <lucas.demarchi@intel.com> > >> > --- > >> > 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. > > and then remove display.mmio_offset since vlv/chv are the only ones > using it? There should tons of other registers still using it.
On Wed, 01 Feb 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Feb 01, 2023 at 09:26:29AM -0800, Lucas De Marchi wrote: >> On Wed, Feb 01, 2023 at 02:09:54PM +0200, Ville Syrjälä wrote: >> >On Wed, Feb 01, 2023 at 11:59:19AM +0200, Jani Nikula wrote: >> >> On Tue, 31 Jan 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> >> > -#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. >> >> and then remove display.mmio_offset since vlv/chv are the only ones >> using it? > > There should tons of other registers still using it. Yeah. It's just that DISPLAY_MMIO_BASE() was sprinkled around gratuitously, when separate VLV/CHV specific macros would have been better options. This is really case by case. Remove the requirement for drm_i915_private access where it makes sense, and pass the pointer explicitly elsewhere. BR, Jani.
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)) /* 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) + \ + _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_USBC1_AUX_CH_DATA1 0x16F214 #define _XELPDP_USBC2_AUX_CH_DATA1 0x16F414 #define _XELPDP_USBC3_AUX_CH_DATA1 0x16F614 #define _XELPDP_USBC4_AUX_CH_DATA1 0x16F814 -#define XELPDP_DP_AUX_CH_DATA(aux_ch, i) _MMIO(_PICK(aux_ch, \ - _DPA_AUX_CH_DATA1, \ - _DPB_AUX_CH_DATA1, \ - 0, /* port/aux_ch C is non-existent */ \ - _XELPDP_USBC1_AUX_CH_DATA1, \ - _XELPDP_USBC2_AUX_CH_DATA1, \ - _XELPDP_USBC3_AUX_CH_DATA1, \ - _XELPDP_USBC4_AUX_CH_DATA1) + (i) * 4) +#define XELPDP_DP_AUX_CH_DATA(aux_ch, i) _MMIO(DISPLAY_MMIO_BASE(dev_priv) + \ + _PICK(aux_ch, \ + _DPA_AUX_CH_DATA1, \ + _DPB_AUX_CH_DATA1, \ + 0, /* port/aux_ch C is non-existent */ \ + _XELPDP_USBC1_AUX_CH_DATA1, \ + _XELPDP_USBC2_AUX_CH_DATA1, \ + _XELPDP_USBC3_AUX_CH_DATA1, \ + _XELPDP_USBC4_AUX_CH_DATA1) + (i) * 4) #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31) #define DP_AUX_CH_CTL_DONE (1 << 30)
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 <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 73 ++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 34 deletions(-)