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

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

On 21.09.2022 21:12, Ville Syrjälä wrote:
> On Wed, Sep 21, 2022 at 10:52:59PM +0530, Balasubramani Vivekanandan 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.
> 
> No one should really be doing that, apart from a few exceptions
> during initialization. Apart from that I don't think enum port
> should really be doing anything else these days than being the
> register block offset we pass to the port registers.

Yes, my main concern is trying to fix the enum values of port to match
the register block offset. As we have seen with ports PORT_D_XELPD,
PORT_E_XELPD, just how if the hardware moves the register offset of a
port to a new position how much chaos it creates in the driver.
It resulted in a 
1. new function xelpd_hpd_pin, 
2. New condition check `if (DISPLAY_VER(dev_priv) >= 13 && port >= PORT_D_XELPD) {` 
   in function intel_ddi_init()
3. New conditional check in intel_port_to_phy() function
4. A new array item in `static const struct intel_ddi_port_domains d13_port_domains[] = {`

All these special handling can be avoided if we were not to fix the enum
values of port to register offset as shown in this series.

I am also worried driver how much mess it would create if the newer platform adds
new TypeC ports at register offset after PORT_E_XELPD or if it moves the 
offset of the existing TypeC ports.

> 
> Well, the VBT code does screw over that idea kinda. I've been
> occasionally pondering some kind of separate namespace for ports
> for the VBT code but haven't really it throught it through in
> any detail.
> 
> > 
> > >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.
> 
> Do we know that it's going to get even more messy?
I see a big possibility. 

> 
> Anyways, we have the exact same thing with AUX CH, so trying to
> change one but not the other isn't really going to help.

Yes, I am aware of it. DDI_BUT_CTL and AUX CH registers have same
relative offset for the ports. My plan is to use the current series as a
prepartion work to clean up the AUX CH handling as well.
I will send a follow up patch for it.

> 
> And on top of that we have the horrorshow in intel_port_to_phy()
> & co. I think the phy stuff is probably what we should try to sort
> out next, since IMO it's the bigger mess.

Agree.

Regards,
Bala

> 
> -- 
> Ville Syrjälä
> Intel