Re: [Intel-gfx] [PATCH v2] drm/i915/display: Don't use port enum as register offset

[Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>]

On 23.09.2022 13:18, Ville Syrjälä wrote:
> On Fri, Sep 23, 2022 at 12:52:48PM +0300, Jani Nikula wrote:
> > On Wed, 21 Sep 2022, Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com> wrote:
> > > Display DDI ports are enumerated as PORT_A,PORT_B... . The enums are
> > > also used as an index to access the DDI_BUF_CTL register for the port.
> > >
> > > With the introduction of TypeC ports, new enums PORT_TC1,PORT_TC2.. were
> > > added starting from enum value 4 to match the index position of the
> > > DDI_BUF_CTL register of those ports. Because those early platforms had
> > > only 3 non-TypeC ports PORT_A,PORT_B, PORT_C followed by TypeC ports.
> > > So the enums PORT_D,PORT_E.. and PORT_TC1,PORT_TC2.. used the same enum
> > > values.
> > >
> > > Driver also used the condition `if (port > PORT_TC1)` to identify if a
> > > port is a TypeC port or non-TypeC.
> > >
> > > From XELPD, additional non-TypeC ports were added in the platform
> > > calling them as PORT D, PORT E and the DDI registers for those ports
> > > were positioned after TypeC ports.  So the enums PORT_D and PORT_E can't
> > > be used as their values do not match with register position. It led to
> > > creating new enums PORT_D_XELPD, PORT_E_XELPD for ports D and E.
> > >
> > > The condition `if (port > PORT_TC1)` was no more valid for XELPD to
> > > identify a TypeC port. Also it led to many additional special checks for
> > > ports PORT_D_XELPD/PORT_E_XELPD.
> > >
> > > With new platforms indicating that the DDI register positions of ports
> > > can vary across platforms it makes no more feasible to maintain the port
> > > enum values to match the DDI register position.
> > >
> > > Port DDI register position is now maintained in a separate datastructure
> > > part of the  platform device info and ports are enumerated independently.
> > > With enums for TypeC ports defined at the bottom, driver can easily
> > > identify the TypeC ports.
> > >
> > > Removed a WARN_ON as it is no longer valid. The WARN was added in
> > > commit - "327f8d8c336d drm/i915: simplify setting of ddi_io_power_domain"
> > > The ddi_io_power_domain calculation has changed completely since the
> > > commit and doesn't need this WARN_ON anymore.
> > >
> > > Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
> > 
> > I agree with the premise that defining platform specific port enums such
> > as PORT_D_XELPD to tackle differences in register offsets is handling
> > the problem at the wrong abstraction level.
> > 
> > I am not (at least not yet) convinced with the approach of adding
> > platform specific mappings in .display.ddi_offsets. The main problem I
> > have with that is adding yet another way to deal with different register
> > offsets. We already have many, and adding a new one isn't appealing.
> > 
> > Not that this *is* different from .display.pipe_offsets and
> > .display.trans_offsets which are actual *offsets*. The solution here is
> > actually misnamed; it's about indexes, not offsets.
> > 
> > Finally, even if we were to choose this approach, this should be split
> > to at least three separate patches. First, pass i915 to the register
> > macro, no other changes, totally non-functional. Second, use the
> > indexes. Third, remove PORT_D_XELPD etc.
> > 
> > I'm still considering alternatives. In the mean time, please find some
> > random comments on the details inline.
> 
> One of the earlier alternatives proposed was some kind of declarative
> struct to describe each port, which would include separate indexes needed
> for different things (among information on the type of DDI/PHY/etc.)
> I think there was some attempt at something like that, but IIRC it
> tried to do a bunch of other stuff too so it got bikeshedded to death.
> 
> I guess one key question is: Do we need to freestanding DDI/AUX/etc.
> register accesses or can we assume the encoder struct is always there?
> That would dictate whether we need any magic in the register macros at
> all, or whether we can just trust the caller to pass in the right
> index.

Wouldn't it be a big restriction to say it wouldn't be possible to read
the DDI registers of a port if it has no encoder struct associated with
it?
AFAIU from the driver, encoder struct would be created for a port, if it
is enabled in the VBT. Can we then assume that if a port is not enabled
in VBT, there would be never a need to read its registers?

> 
> Oh, and the other key question is: Is an index enough, or are the
> register offsets going to get really random at some point?

Looking at the DDI register offsets through platform generations and
also at other parts of hardware where register series exists, it seems
hardware tends to keep the increments between register offsets fixed,
though it may change the size of increment.
So I think we can assume the index would be sufficient.

Regards,
Bala

> 
> So far I'm not aware of any changes the status-quo in the forseeable
> future, so not really seeing this part as super urgent compared to
> the whole PHY mess, which has been much more volatile in recent
> times.
> 
> -- 
> Ville Syrjälä
> Intel