Re: [Intel-gfx] [PATCH 2/2] drm/i915: Move common mmio base out of private macros

[Jani Nikula <jani.nikula@linux.intel.com>]

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.


-- 
Jani Nikula, Intel Open Source Graphics Center