[Intel-gfx] [PATCH 1/3] drm/i915/tgl+: Add locking around DKL PHY register accesses

[Jani Nikula]

On Wed, 19 Oct 2022, Imre Deak <imre.deak at intel.com> wrote:
> On Wed, Oct 19, 2022 at 03:30:58PM +0300, Jani Nikula wrote:
>> On Wed, 19 Oct 2022, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
>> > On Wed, Oct 19, 2022 at 12:19:49PM +0300, Ville Syrjälä wrote:
>> >> On Wed, Oct 19, 2022 at 12:00:02PM +0300, Jani Nikula wrote:
>> >> > On Tue, 18 Oct 2022, Imre Deak <imre.deak at intel.com> wrote:
>> >> > > Accessing the TypeC DKL PHY registers during modeset-commit,
>> >> > > -verification, DP link-retraining and AUX power well toggling is racy
>> >> > > due to these code paths being concurrent and the PHY register bank
>> >> > > selection register (HIP_INDEX_REG) being shared between PHY instances
>> >> > > (aka TC ports) and the bank selection being not atomic wrt. the actual
>> >> > > PHY register access.
>> >> > >
>> >> > > Add the required locking around each PHY register bank selection->
>> >> > > register access sequence.
>> >> > 
>> >> > I honestly think the abstraction here is at a too low level.
>> >> > 
>> >> > Too many places are doing DKL PHY register access to begin with. IMO the
>> >> > solution should be to abstract DKL PHY better, not to provide low level
>> >> > DKL PHY register accessors.
>> >> > 
>> >> > Indeed, this level of granularity leads to a lot of unnecessary
>> >> > lock/unlock that could have a longer span otherwise, and the interface
>> >> > does not lend itself for that.
>> >> 
>> >> It's no worse than uncore.lock. No one cares about that in
>> >> these codepaths either.
>> >> 
>> >> > Also requires separate bank selection for
>> >> > every write, nearly doubling the MMIO writes.
>> >> 
>> >> Drop in the ocean. This is all slow modeset stuff anyway.
>> >> 
>> >> IMO separate reg accessors is the correct way to handle indexed
>> >> registers unless you have some very specific performance concerns
>> >> to deal with.
>> 
>> Fair enough.
>> 
>> > Now, whether those accessors need to be visible everywere is another
>> > matter. It should certainly be possible to suck all dkl phy stuff
>> > into one file and keep the accessors static. But currently eveything
>> > is grouped by function (PLLs in one file, vswing stuff in another,
>> > etc.). We'd have to flip that around so that all the sub functions
>> > of of each IP block is in the same file. Is that a better apporach?
>> > Not sure.
>> 
>> I'm often interested in the precedent a change makes. What's the
>> direction we want to go to?
>> 
>> So here, we're saying the DKL PHY registers are special, and need to be
>> accessed via dedicated register accessors. To enforce this, we create
>> strong typing for DKL PHY registers. We go out of our way to make it
>> safe to access DKL PHY registers anywhere in the driver.
>> 
>> Do we want to add more and more register types in the future? And
>> duplicate the accessors for each? Oops, looks like we're already on our
>> way [1].
>
> Making the DKL PHY accesses type safe was just a bonus, the main reason
> for adding the dkl_phy_reg struct (in a later refactoring patch) is that
> defining those registers only makes sense to me specifying all the
> attributes (both MMIO and the bank index) of the register at one place.
> That's instead of the current way of having to pass these separately to
> each accessor functions. For instance to be able to call
>
> read(DKL_PLL_DIV0(tc_port))
>
> instead of having to remember the index of each (non lane-instanced)
> register and call
>
> read(DKL_PLL_DIV0(tc_port), 2)
>
> It also makes more sense to me that the register itself is parametric
> if that's needed (lane-instanced registers), for instance
>
> read(DKL_TX_DPCNTL0(tc_port, 0))
>
> instead of this being a separate parameter of each accessor function:
>
> read(DKL_TX_DPCNTL0(tc_port), 0)

This is actually a very good point.

An alternative to this that I've been pondering separately, before these
patches, is expanding i915_reg_t to encode things like "display
register", "mcr register", etc.

So you'd still have only one i915_reg_t type, and only one set of
accessors, but they could be smarter behind the scenes. But I don't like
teaching intel uncore about stuff like dkl either. And the main point
would be avoiding all the duplication that C type safety requires.

But it's a moot point anyway because we also need something to backport
to stable, and I acknowledge your approach makes a lot of sense for that
too.

>> My argument is that maybe access to such a hardware block should instead
>> be limited to a module (.c file) that abstracts the details. Abstract
>> hardware blocks and function, not registers. How many files need big
>> changes to add a new PHY?
>
> I think the accessors could be added to a new intel_tc_phy.c file
> instead? (That would still allow further refactoring of both the MG and
> DKL functionality as a follow-up to this change for -stable.)

So, why intel_tc_anything? Why not just intel_dkl_phy.[ch],
intel_dkl_phy_regs.h? Even if initially limited to the register
accessors, you could easily move things like
tgl_dkl_phy_set_signal_levels() there, just like
intel_snps_phy_set_signal_levels() is in intel_snps_phy.c. And you could
have intel_mg_phy.c for MG stuff.

I guess intel_tc_phy_regs.h would mostly be split to
intel_dkl_phy_regs.h and intel_mg_phy_regs.h.


BR,
Jani.


>
>> BR,
>> Jani.
>> 
>> [1] https://lore.kernel.org/r/20221014230239.1023689-13-matthew.d.roper@intel.com
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center