From: Jani Nikula <jani.nikula@linux.intel.com>
To: Lucas De Marchi <lucas.demarchi@intel.com>,
intel-gfx@lists.freedesktop.org
Cc: Lucas De Marchi <lucas.demarchi@intel.com>,
Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Move common mmio base out of private macros
Date: Wed, 01 Feb 2023 11:59:19 +0200 [thread overview]
Message-ID: <878rhh3dvs.fsf@intel.com> (raw)
In-Reply-To: <20230131191542.1695398-2-lucas.demarchi@intel.com>
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)
--
Jani Nikula, Intel Open Source Graphics Center
next prev parent reply other threads:[~2023-02-01 9:59 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-31 19:15 [Intel-gfx] [PATCH 1/2] drm/i915: Split _MMIO() for _PORT3()/_PIPE3() Lucas De Marchi
2023-01-31 19:15 ` [Intel-gfx] [PATCH 2/2] drm/i915: Move common mmio base out of private macros Lucas De Marchi
2023-02-01 9:59 ` Jani Nikula [this message]
2023-02-01 12:09 ` Ville Syrjälä
2023-02-01 17:26 ` Lucas De Marchi
2023-02-01 17:39 ` Ville Syrjälä
2023-02-02 10:11 ` Jani Nikula
2023-01-31 20:03 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Split _MMIO() for _PORT3()/_PIPE3() Patchwork
2023-01-31 21:55 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=878rhh3dvs.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=lucas.demarchi@intel.com \
--cc=rodrigo.vivi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox